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

Feature flags #4940

Merged
merged 11 commits into from
May 6, 2018
Merged

Conversation

ridiculousfish
Copy link
Member

@ridiculousfish ridiculousfish commented Apr 25, 2018

This is my proposal for feature flags, as a mechanism for introducing breaking changes ("features"). This may include removing existing syntax, or introducing new incompatible syntax.

Feature flags are set at startup via the command line and variables (environment or universal), and cannot be changed after: you have to make a new session. They can be queried at any point in the code. Features are also grouped. For example, we would have a "3.0" group for features introduced in 3.0; this lets authors test for 3.0 compatibility in one go. The "all" group is bleeding edge.

Feature flags are only intended to be used for transitions. The "off" state is the old behavior, the "on" state is the new behavior, and in a few releases we'll remove support for running with the feature off. They are not a long-term configuration mechanism.

This is how we would use feature flags to introduce a breaking change:

  1. Add a feature flag entry. This entry contains a short feature name, and one or more group names. For example, the feature stderr-nocaret is in group 3.0. The feature defaults to off, but we'll publish the upcoming change, and its feature flag name.
  2. Script authors can test their script against the new feature by passing it on the command line: fish --features stderr-nocaret or fish --features 3.0
  3. Users can opt into new features by setting a universal variable. set -U fish_features stderr-nocaret.
  4. status can be used to list features, and check if they are on or off (should rarely be necessary).
  5. After a bake-in period, we'll switch the default to on. During this period the feature can still be disabled: fish --features no-stderr-nocaret.
  6. After another period, we'll remove support for the old behavior outright, and mark the feature as archived (still testable, but no longer reported).

All functions and completions that ship with fish need to be compatible with any feature combination, as much as is possible.

This PR introduces the feature flag mechanism, and adds a feature flag for the stderr-nocaret behavior, defaulting to off. That is, carets are still redirections, but users can opt into the new behavior where carets are ordinary characters.

This PR contains the machinery and some tests. I'm eager to hear feedback on this approach before moving forwards with docs, etc.

@faho
Copy link
Member

faho commented Apr 25, 2018

This sounds like a brilliant idea, and I'd love it for % and ? as well for 3.0.

status being hooked up would allow e.g. deprecating . for source this way, as it's a pure-script feature.

I don't think we need to do it for any of the other changes.

Would it be possible to add a link to further documentation to the description? Or hook it up to help, so you'd do help feature std-noerr or something, and that gives you a paragraph or so explaining the change?

features_t::flag_t flag;

/// User-presentable short name of the feature flag.
const wchar_t *name;
Copy link
Member

Choose a reason for hiding this comment

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

Any reason these are wchar_t instead of wcstring?

Copy link
Member Author

Choose a reason for hiding this comment

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

There will be string literals, so it's a little cheaper to initialize since it can be in a read-only segment, and doesn't need malloc().

@faho
Copy link
Member

faho commented Apr 25, 2018

I'd love it for % and ? as well for 3.0.

One issue with ? specifically though is that we'd have to jump through hoops to make e.g. our git completions work in both modes, because there's no nice way to make a case that matches just a literal ?? in both.

The best I can come up with is set -l dq '??' and later case "$dq".

Also can fish -n be taught about this?

@zanchey
Copy link
Member

zanchey commented Apr 29, 2018

If I'm distributing a fish script, is there a way for me to set the feature flags I want for my script? The shebang line isn't much good because only the first argument is useful.

@faho
Copy link
Member

faho commented Apr 29, 2018

If I'm distributing a fish script, is there a way for me to set the feature flags I want for my script?

If I'm understanding @ridiculousfish correctly, the idea is that you wouldn't - if you're touching the script, you should just port it to where it doesn't need "feature" flags (I think "compatibility" flags might be a better name) instead of adding any flags - that would become obsolete soon.

@ridiculousfish
Copy link
Member Author

Yeah that's the hope. Ideally writing a script that works on multiple versions is not hard. If it is hard then the features were badly designed.

@ridiculousfish
Copy link
Member Author

One issue with ? specifically though is that we'd have to jump through hoops to make e.g. our git completions work in both modes, because there's no nice way to make a case that matches just a literal ?? in both.

Nice spotting. This is annoying enough that we should just continue to support '?' in all cases. That is, '\?' will always be a literal ? regardless of whether ? is a glob. This feels harmless to me.

Also can fish -n be taught about this?

What did you have in mind? The feature flags don't make any particular syntax an error, they just change its meaning.

@faho
Copy link
Member

faho commented Apr 29, 2018

That is, '?' will always be a literal ? regardless of whether ? is a glob.

That might be surprising when it comes to regexes - ? has a special meaning there as well.

I'd rather just case do that, for the time being.

What did you have in mind?

Warn when one of these features is used.

E.g. with the following script:

somecommand ^/dev/null

run fish -n or maybe fish -n --features stderr-nocaret on it, and get:

"Line XY: Use of "^" where it would mean stderr
somecommand ^/dev/null
                        ^

as a sort of static analysis to aid in porting away from deprecated features.

See e.g. #4950 - we didn't catch the remaining caret redirection because it had a space, and I think all of us just used ad-hoc regexes to check for them.

This introduces a new type features_t that exposes feature flags. The intent
is to allow a deprecation/incremental adoption path. This is not a general
purpose configuration mechanism, but instead allows for compatibility during
the transition as features are added/removed.

Each feature has a user-presentable short name and a short description. Their
values are tracked in a struct features_t.

We start with one feature stderr_nocaret, but it's not hooked up yet.
This teaches the status command to work with features.
'status features' will show a table listing all known features and whether
they are currently on or off.
`status test-feature` will test an individual feature, setting the exit status to
0 if the feature is on, 1 if off, 2 if unknown.
This introduces a new command line option --features which can be used for
enabling or disabling features for a particular fish session.

Examples:
  fish --features stderr-nocaret
  fish --features 3.0,no-stderr-nocaret
  fish --features all

Note that the feature set cannot be changed in an existing session.
This partially reverts 5b489ca, with
carets acting as redirections unless the stderr-nocaret flag is set.
This flag is off by default but may be enabled on the command line:

fish --features stderr-nocaret
This enables users to opt in (or out) of specific features by setting
the fish_features environment variable.

For example `set -U fish_features stderr-nocaret` to opt into removing the
caret redirection.
This adds a feature flag for controlling whether question marks are globs.
It is not yet hooked up.
This partially reverts 6e56637 and fish-shell#4520
by bringing back the ? wildcard, guarded by the qmark-noglob feature flag.
This is a convenience over fish_features().test()
ridiculousfish added a commit to ridiculousfish/fish-shell that referenced this pull request May 6, 2018
This merges support for feature flags.

Closes fish-shell#4940
@ridiculousfish ridiculousfish merged commit 060643a into fish-shell:master May 6, 2018
@zanchey zanchey added this to the fish-3.0 milestone Sep 26, 2018
@ridiculousfish ridiculousfish deleted the futurefeatures branch December 13, 2019 03:53
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants