Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Recursively export constants from modules #10049

Merged
merged 15 commits into from Aug 20, 2023
Merged

Conversation

kubouch
Copy link
Contributor

@kubouch kubouch commented Aug 18, 2023

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
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 kubouch marked this pull request as ready for review August 19, 2023 19:58
@kubouch
Copy link
Contributor Author

kubouch commented Aug 19, 2023

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.

Copy link
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 😏

tests/const_/mod.rs Show resolved Hide resolved
@amtoine
Copy link
Member

amtoine commented Aug 20, 2023

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

@kubouch
Copy link
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
Member

amtoine commented Aug 20, 2023

thanks for answering my thread @kubouch 🙏

@WindSoilder
Copy link
Collaborator

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

@amtoine
Copy link
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
19 checks passed
@kubouch kubouch deleted the const-recursive branch August 20, 2023 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants