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

Allow --unstable to be set with an environment variable #1588

Merged
merged 13 commits into from
Oct 9, 2023

Conversation

neunenak
Copy link
Contributor

Allow the environment variable JUST_ALLOW_UNSTABLE=true to act as equivalent to the --unstable flag. Any other value of JUST_ALLOW_UNSTABLE is treated as false.

This addresses #1575

Copy link
Owner

@casey casey left a comment

Choose a reason for hiding this comment

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

Just a few random nitpicks, see comments. Also: Is there any sort of convention regarding how other tools convert environment variable values are converted to bools? For example, Rust has the RUST_BACKTRACE variable, which if unset or set to 0 will not capture a backtrace, but if set to any other value will. I wonder if there's any particular reason to go with one standard vs another.

src/config.rs Outdated
@@ -3,6 +3,8 @@ use {
clap::{App, AppSettings, Arg, ArgGroup, ArgMatches, ArgSettings},
};

pub(crate) const UNSTABLE_KEY: &str = "JUST_ALLOW_UNSTABLE";
Copy link
Owner

Choose a reason for hiding this comment

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

What about JUST_UNSTABLE instead? JUST_UNSTABLE=true makes sense, and is shorter.

src/config.rs Outdated
@@ -569,6 +571,9 @@ impl Config {
None
};

let unstable = matches.is_present(arg::UNSTABLE)
|| std::env::var_os(UNSTABLE_KEY).map_or(false, |val| val.to_string_lossy() == "true");
Copy link
Owner

Choose a reason for hiding this comment

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

I'd be inclined to inline this:

Suggested change
|| std::env::var_os(UNSTABLE_KEY).map_or(false, |val| val.to_string_lossy() == "true");
|| std::env::var_os("JUST_UNSTABLE").map_or(false, |val| val.to_string_lossy() == "true");

@neunenak
Copy link
Contributor Author

Just a few random nitpicks, see comments. Also: Is there any sort of convention regarding how other tools convert environment variable values are converted to bools? For example, Rust has the RUST_BACKTRACE variable, which if unset or set to 0 will not capture a backtrace, but if set to any other value will. I wonder if there's any particular reason to go with one standard vs another.

I'm not aware of any convention other than what seems reasonable from the perspective of a developer working with standard programming languages. I'd probably allow both "0" and "false" to be treated as a false boolean, the convention that setting the env var to anything else makes it true, seems reasonable to me.

neunenak added 3 commits June 15, 2023 00:35
Allow the environment variable JUST_ALLOW_UNSTABLE=true to act as
equivalent to the --unstable flag. Any other value of
JUST_ALLOW_UNSTABLE is treated as false.
@neunenak neunenak requested a review from casey June 15, 2023 07:59
@casey casey enabled auto-merge (squash) October 9, 2023 03:45
@casey casey merged commit 992c694 into casey:master Oct 9, 2023
5 checks passed
@casey
Copy link
Owner

casey commented Oct 9, 2023

Tweaked a little bit and merged. Sorry for letting this sit for so long. In the mean time, --dump-format json was stabilized, so I changed the test to use --fmt instead.

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.

2 participants