Skip to content

Recursively export constants from modules#10049

Merged
amtoine merged 15 commits intonushell:mainfrom
kubouch:const-recursive
Aug 20, 2023
Merged

Recursively export constants from modules#10049
amtoine merged 15 commits intonushell:mainfrom
kubouch:const-recursive

Conversation

@kubouch
Copy link
Copy Markdown
Contributor

@kubouch kubouch commented Aug 18, 2023 •
edited
Loading

Description

#9773 introduced constants to modules and allowed to export them, but only within one level. This PR:

  • allows recursive exporting of constants from all submodules
  • fixes submodule imports in a list import pattern
  • makes sure exported constants are actual constants

Should unblock #9678

Example:

module spam {
    export module eggs {
        export module bacon {
            export const viking = 'eats'
        }
    }
}

use spam 
print $spam.eggs.bacon.viking  # prints 'eats'

use spam [eggs]
print $eggs.bacon.viking  # prints 'eats'

use spam eggs bacon viking
print $viking  # prints 'eats'

Limitation 1:

Considering the above spam module, attempting to get eggs bacon from spam module doesn't work directly:

use spam [ eggs bacon ]  # attempts to load `eggs`, then `bacon`
use spam [ "eggs bacon" ]  # obviously wrong name for a constant, but doesn't work also for commands

Workaround (for example):

use spam eggs
use eggs [ bacon ]

print $bacon.viking  # prints 'eats'

I'm thinking I'll just leave it in, as you can easily work around this. It is also a limitation of the import pattern in general, not just constants.

Limitation 2:

overlay use successfully imports the constants, but overlay hide does not hide them, even though it seems to hide normal variables successfully. This needs more investigation.

User-Facing Changes

Allows recursive constant exports from submodules.

Tests + Formatting

After Submitting

@kubouch kubouch changed the title Recursively constants from modules Recursively export constants from modules Aug 18, 2023
kubouch added 6 commits August 19, 2023 19:35
@kubouch
Fix capturing consts in modules
888c2c9 
This might be a reason why regular `let` might be problematic in
modules. In the case of `const`, we can grab the value directly from
the parsed results, but with `let`, we might need to evaluate the
module.
@kubouch
Allow importing constants using overlay use
f92c00b
@kubouch
Remove debug prints
628c865
@kubouch
Add a separate method for fetching constants
73d21fa
@kubouch
Add tests
2d87b8f
@kubouch
Fix nested list imports
62883b6
@kubouch kubouch marked this pull request as ready for review August 19, 2023 19:58
@kubouch
Copy link
Copy Markdown
Contributor Author

kubouch commented Aug 19, 2023 •
edited
Loading

Just discovered that when you import constants with overlay use, they won't be hidden when you call overlay hide. I'm not sure why. I think I'll remove the overlay use support for constants until it gets resolved.

amtoine
amtoine reviewed Aug 20, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@amtoine amtoine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks like a very good PR to me, very good job 👌

and i love your examples 🙏 ❤️

i just have a question about one test 😏

@amtoine
Copy link
Copy Markdown
Member

amtoine commented Aug 20, 2023

also, maybe we should wait for after the release at this point?

@amtoine amtoine mentioned this pull request Aug 20, 2023
Merged
@kubouch
Copy link
Copy Markdown
Contributor Author

kubouch commented Aug 20, 2023

I'd personally include it in the release, since the original export const implementation had some rough edges that are easy to encounter. We could ship with a more polished experience.

@amtoine
Copy link
Copy Markdown
Member

amtoine commented Aug 20, 2023

thanks for answering my thread @kubouch 🙏

@WindSoilder
Copy link
Copy Markdown
Contributor

WindSoilder commented Aug 20, 2023

Thanks! LGTM! I've tried and played with it, and it works pretty good!

@amtoine
Copy link
Copy Markdown
Member

amtoine commented Aug 20, 2023

let's land this before we freeze for the release for real 🥳

@amtoine amtoine merged commit 3148acd into nushell:main Aug 20, 2023
@kubouch kubouch deleted the const-recursive branch August 20, 2023 13:58
@blindFS blindFS mentioned this pull request Jan 26, 2025
Merged
kubouch pushed a commit that referenced this pull request Jan 26, 2025
@blindFS
refactor(parser): use var_id for most constants in `ResolvedImportPat…
f46f8b2 
…tern` (#14920)

# Description

This PR replaces most of the constants in `ResolvedImportPattern` from
values to VarIds, this has benefits of:
1. less duplicated variables in state
2. precise span of variable, for example when calling `goto def` on a
const imported by the `use` command, this allows it to find the original
definition, instead of where the `use` command is.

Note that the logic is different here for nested submodules, not all
values are flattened and propagated to the outmost record variable, but
I didn't find any differences in real world usage.

I noticed that it was changed from `VarId` to `Value` in #10049.
Maybe @kubouch can find some edge cases where this PR fails to work as
expected.

In my view, the record constants for `ResolvedImportPattern` should even
reduced to single entry, if not able to get rid of.

# User-Facing Changes

# Tests + Formatting

# After Submitting
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@amtoine amtoine amtoine left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@kubouch @amtoine @WindSoilder