This repository was archived by the owner on Apr 14, 2021. It is now read-only.
Merged
Conversation
This setting is not meant to be used by end users. The `viz` command is going away and we plan to fully remove the code once we only have to maintain bundler 3 or higher versions. Adding a setting makes it look like the presence of this command is something "configurable", but it's not.
This setting is not meant to be used by end users. The `console` command is going away and we plan to fully remove the code once we only have to maintain bundler 3 or higher versions. Adding a setting makes it look like the presence of this command is something "configurable", but it's not.
deivid-rodriguez
force-pushed
the
remove_command_removal_settings
branch
from
6215b84 to
714d2c7
Compare
ghost
pushed a commit
that referenced
this pull request
Apr 4, 2019
7084: Remove command removal settings r=indirect a=deivid-rodriguez ### What was the end-user problem that led to this PR? The problem was that the `viz_command` and `console_command` settings are not meant to be something configured by users. These commands are going away, we don't want to support optionally keeping them. ### What was your diagnosis of the problem? My diagnosis was that we are using settings for two different things: * Different supported behaviors that are configurable by users. * Feature flags that allow us developers to easily maintain a single branch of code and making breaking changes more easily, and users to opt-in/out of certain new features/changes. The second case is valid but it's complicated because we need to commit to provide a proper life cycle for the setting: * Allow opting in through the setting. * Toggle the setting's default value. * Deprecate the value for the setting enabling the old behavior. * Deprecate the setting altogether making it a no-op. * Finally removing the setting. I plan to work on that, but I didn't start yet. Instead, what we've been doing to workaround this is to try to not expose these settings to users by, for example, not documenting them. However, in my opinion, in this particular case it's best to instead not provide a setting at all, and check for the bundler version directly for whether we should be providing the command. The only extra benefit of providing a feature flag, namely, allowing opting in early to the new behavior, is not necessary here because users can "opt in" anyways by simply not using the deprecated commands. ### What is your fix for the problem, implemented in this PR? My fix is to remove the settings and check for `Bundler.feature_flag.bundler_3_mode?` instead. ### Why did you choose this fix out of the possible options? I chose this fix because it's simple and it makes it clear that keeping these commands and not something we want to support. They are going away for good. Co-authored-by: David Rodríguez <deivid.rodriguez@riseup.net>
Build succeeded |
ghost
merged commit
This pull request was closed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What was the end-user problem that led to this PR?
The problem was that the
viz_commandandconsole_commandsettings are not meant to be something configured by users. These commands are going away, we don't want to support optionally keeping them.What was your diagnosis of the problem?
My diagnosis was that we are using settings for two different things:
The second case is valid but it's complicated because we need to commit to provide a proper life cycle for the setting:
I plan to work on that, but I didn't start yet.
Instead, what we've been doing to workaround this is to try to not expose these settings to users by, for example, not documenting them.
However, in my opinion, in this particular case it's best to instead not provide a setting at all, and check for the bundler version directly for whether we should be providing the command. The only extra benefit of providing a feature flag, namely, allowing opting in early to the new behavior, is not necessary here because users can "opt in" anyways by simply not using the deprecated commands.
What is your fix for the problem, implemented in this PR?
My fix is to remove the settings and check for
Bundler.feature_flag.bundler_3_mode?instead.Why did you choose this fix out of the possible options?
I chose this fix because it's simple and it makes it clear that keeping these commands is not something we want to support. They are going away for good.