Add infrastructure for experimental options#16028
Conversation
Bahex
left a comment
There was a problem hiding this comment.
Sorry for taking so long to review this 🙇
Co-authored-by: Bahex <Bahex@users.noreply.github.com>
|
@Bahex that should be it |
|
Looks great! The only thing I'm not sure about here is if we would want to remove experimental options after implementing them as the default behavior. I'm mostly thinking about the use-case where we would want to have some way for user code to assert the presence of an experimental option, like: @requires-experimental-option-bikeshed-name "newbehavior"
def my-cool-command [] {
# some code which depends on `newbehavior`, parse error if `newbehavior` is disabled
}My concern here is that if we adopted Maybe we could keep a separate list of recognized experimental options which have been implemented as default? |
We could do that, but do we want this as part of this PR? This PR also doesn't include the way for scripts requiring an experimental option. |
|
If we think the "list of recognized options" is a good approach then I'm all good to merge this, just wanted to make sure we wouldn't need any more fundamental changes to our approach to accommodate the experimental option attribute |
|
I think it is a reasonable assumption that if you use "experimental" things you are OK with dealing with those deprecations, stabilizing the list of experimental options feels like investing in the wrong bits of stability promises. |
|
Could it not just be added as a status? Like "redundant" or "implemented" |
# Description In #16028 I also added a test to check that identifiers are valid to ensure that we have consistency there. But I only checked for alphanumeric strings as identifiers. It doesn't allow underscores or dashes. @Bahex used in his PR about #15682 a dash to separate words. So expanded the test to allow that. # User-Facing Changes None. # Tests + Formatting The `assert_identifiers_are_valid` now allows dashes. # After Submitting The tests in #15682 should work then.
|
In preparation for the next release, I think some of our make_release scripts in nu_scripts may need to be changed in order to publish a new crate. If you have time, can you look into that @cptpiepmatz? |
Yeah, I can do that |
# Description In nushell#16028 I also added a test to check that identifiers are valid to ensure that we have consistency there. But I only checked for alphanumeric strings as identifiers. It doesn't allow underscores or dashes. @Bahex used in his PR about nushell#15682 a dash to separate words. So expanded the test to allow that. # User-Facing Changes None. # Tests + Formatting The `assert_identifiers_are_valid` now allows dashes. # After Submitting The tests in nushell#15682 should work then.
<!-- if this PR closes one or more issues, you can automatically link the PR with them by using one of the [*linking keywords*](https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword), e.g. - this PR should close #xxxx - fixes #xxxx you can also mention related issues, PRs or discussions! --> # Description <!-- Thank you for improving Nushell. Please, check our [contributing guide](../CONTRIBUTING.md) and talk to the core team before making major changes. Description of your pull request goes here. **Provide examples and/or screenshots** if your changes affect the user experience. --> This PR changes the behavior of #16028 to allow enabling experimental options even if they are marked as deprecated. # Tests + Formatting <!-- Don't forget to add tests that cover your changes. Make sure you've run and fixed any issues with these commands: - `cargo fmt --all -- --check` to check standard code formatting (`cargo fmt --all` applies these changes) - `cargo clippy --workspace -- -D warnings -D clippy::unwrap_used` to check that you're using the standard code style - `cargo test --workspace` to check that all tests pass (on Windows make sure to [enable developer mode](https://learn.microsoft.com/en-us/windows/apps/get-started/developer-mode-features-and-debugging)) - `cargo run -- -c "use toolkit.nu; toolkit test stdlib"` to run the tests for the standard library > **Note** > from `nushell` you can also use the `toolkit` as follows > ```bash > use toolkit.nu # or use an `env_change` hook to activate it automatically > toolkit check pr > ``` --> - 🟢 `toolkit fmt` - 🟢 `toolkit clippy` - 🟢 `toolkit test` - 🟢 `toolkit test stdlib`
Description
In this PR I introduce infrastructure for handling experimental options (aka feature flags) across our codebase, designed for low-level use.
The goal is to make it dead simple to gate major changes (like swapping out the parser or altering evaluation behavior) behind a static toggle. These options are available anywhere in the code and are set very early during initialization.
An
ExperimentalOptionis just a fancy global boolean. You use.get()to read it, and it behaves similarly to what you'd expect from a Cargo feature, just at runtime.These options are meant to stay fixed after initialization. They're not dynamic and should only be changed at startup.
Options can be enabled via:
NU_EXPERIMENTAL_OPTIONSenvironment variable--experimental-optionsCLI flag (e.g.nu --experimental-options=[example])Why "experimental options"?
I went with "experimental options" instead of "feature flags" to avoid confusion with Cargo features.
Each option also carries a
StabilityStatusvalue so we can communicate to users how stable or likely to change a feature is.Error handling
If option parsing fails, we show proper warnings to make it obvious what went wrong:
Implementation
The implementation lives in a new crate so it can be used from anywhere in the workspace.
User-Facing Changes
debug experimental-optionscommandversioncommandNU_EXPERIMENTAL_OPTIONSor--experimental-optionsTests + Formatting
I added a test to check that all experimental options are well-formed.
After Submitting
We can use this system in @Bahex’s PR #15682.