Skip to content
This repository has been archived by the owner. It is now read-only.

Turn on deprecations by default #6965

Merged
8 commits merged into from Feb 22, 2019
Merged

Turn on deprecations by default #6965

8 commits merged into from Feb 22, 2019

Conversation

deivid-rodriguez
Copy link
Member

@deivid-rodriguez deivid-rodriguez commented Feb 14, 2019

What was the end-user problem that led to this PR?

The problem was we've had deprecations in place for a long time, but we've never displayed them to users by default.

What was your diagnosis of the problem?

My diagnosis was that the current strategy doesn't work because:

  • Printing deprecations as an optin feature almost never get enabled, so most users don't know about the stuff we'll be deprecating in the future.

  • Printing deprecations in "deprecation releases" doesn't work well either, because it's unclear how long the deprecation release should last and thus how long we need to hold the final release (that will inhibit installation of the deprecation release).

What is your fix for the problem, implemented in this PR?

My fix is to remove the concept of deprecation releases, and to add a feature flag for printing major deprecations that it's enabled by default.

As a extra related change, I also reworded the deprecation messages, because I find the current message "[DEPRECATED FOR 2.0] " a bit confusing because it's unclear what the version printed is referring to (deprecation horizon? current running version?), so I changed it to just "[DEPRECATED] ".

Why did you choose this fix out of the possible options?

I chose this fix because it (once released) finally makes it so that users will know about our deprecations.

@deivid-rodriguez
Copy link
Member Author

deivid-rodriguez commented Feb 14, 2019

@indirect @segiddins You ok with this path forward? I know this was not the original plan, but I read #6586 (comment) and #4855 (comment) and I think we all agree about this now?

@colby-swandale
Copy link
Member

colby-swandale commented Feb 18, 2019

It's normal for users to have many versions of Bundler installed, so i find the deprecation message with the version number pretty handy to know which major version of Bundler has deprecated/removed what.

@@ -44,6 +44,7 @@ def self.settings_method(name, key, &default)
settings_flag(:init_gems_rb) { bundler_2_mode? }
settings_flag(:list_command) { bundler_2_mode? }
settings_flag(:lockfile_uses_separate_rubygems_sources) { bundler_2_mode? }
settings_flag(:major_deprecations) { bundler_2_mode? }
Copy link
Member

@colby-swandale colby-swandale Feb 18, 2019

Choose a reason for hiding this comment

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

IMO this should not be a feature flag as this is not enabling any particular Bundler feature and would also never be removed either. Can we just set the config option to have a default value instead?

Copy link
Member Author

@deivid-rodriguez deivid-rodriguez Feb 18, 2019

Choose a reason for hiding this comment

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

That makes sense to me, I'm going to change it as you suggest 👍.

@deivid-rodriguez
Copy link
Member Author

deivid-rodriguez commented Feb 18, 2019

It's normal for users to have many versions of Bundler installed, so i find the deprecation message with the version number pretty handy to know which major version of Bundler has deprecated/removed what.

It was confusing for me because the first time I saw it I didn't know whether it meant the deprecation horizon (when the feature will be removed), the currently running version, or the version that removed the feature.

Anyways, I don't really find the version that deprecated a feature very useful for users, but I can restore it if others feel differently. If I do restore it, should I at least remove the trailing ".0"? If we deprecate a feature on bundler 2.1, won't that be incorrect?

@deivid-rodriguez
Copy link
Member Author

deivid-rodriguez commented Feb 18, 2019

By the way, thanks so much for having a look at this @colby-swandale !

Copy link
Member

@indirect indirect left a comment

I agree that just turning it on seems reasonable, and the "FOR 2.0" part was not clear.

As for the setting, maybe we can make it always default to on, with a setting named silence_deprecations that you can set to true if you want to hide deprecations?

@deivid-rodriguez
Copy link
Member Author

deivid-rodriguez commented Feb 19, 2019

Ok, so it turns out the feature flag is needed so that we can keep the 1.x specs passing. The way we've been using feature flags is so that we can virtually maintain a single branch, and the only difference between branches is the running bundler version (which triggers a different set of specs).

Enabling deprecations inconditionally made the deprecations be also enabled in bundler 1, and that causes tests to fail. Doing this revealed a few problems with our tests and deprecations that we should address though, so tentatively removing the feature flag was a good thing 😃.

In particular, our tests are using deprecated behaviour. If we don't use the new behavior ourselves, how can we encourage users to do it? 😅. I'll be addressing this on a separate PR, so that our tests always use preferred non deprecated behaviour unless we are testing the deprecations themselves.

Creating a PR to fix the above also surfaced a few problems with some of the deprecations themselves. I'll create issues about these problems that I found as well.

As per what to do with this PR. I see several options:

  • Restore the feature flag, at least for the time being.
  • Remove the 1.x build, and get ready to maintain several branches. Our feature flag strategy didn't fully work after all, and we're maintaining several branches anyways.
  • Hold the PR until I address the above issues.

@deivid-rodriguez
Copy link
Member Author

deivid-rodriguez commented Feb 19, 2019

Oh, another idea: explicitly toggle off deprecations via environment for the 1.x build.

@indirect
Copy link
Member

indirect commented Feb 19, 2019

Adding those updates to the tests sounds great! Also turning off deprecations for the versions before the deprecations are enabled sounds good to me. :)

@deivid-rodriguez
Copy link
Member Author

deivid-rodriguez commented Feb 20, 2019

Also turning off deprecations for the versions before the deprecations are enabled sounds good to me.

Ok, so that means turning them off via environment as suggested in #6965 (comment), right? Just double checking because... well, that is essentially what the feature flag was doing, so maybe I can just restore that.

As for the setting, maybe we can make it always default to on, with a setting named silence_deprecations that you can set to true if you want to hide deprecations?

Regarding this, it sounds like a good idea because it's great naming, but it implies introducing yet another setting to do the same thing. Would we remove (or deprecate 😱) the current setting, or just add it as a "nice redundancy"?

@indirect
Copy link
Member

indirect commented Feb 20, 2019

that means turning them off via environment as suggested in #6965 (comment), right?

Yup, that seems super reasonable to me. 👍

Would we remove (or deprecate 😱) the current setting, or just add it as a "nice redundancy"?

I would be okay with either renaming the setting or adding it as the "user-facing" name for this setting, to provide a better user experience.

@deivid-rodriguez deivid-rodriguez force-pushed the deprecation_changes branch 4 times, most recently from 56384b8 to 227007f Compare Feb 21, 2019
@deivid-rodriguez
Copy link
Member Author

deivid-rodriguez commented Feb 21, 2019

I changed the name of the setting to silence_deprecations, and regarding the failing 1.x specs, it was due to a bug in deprecations that I fixed 👉 cf7fa53.

@deivid-rodriguez
Copy link
Member Author

deivid-rodriguez commented Feb 21, 2019

@indirect May I ask for a final look here?

Including the version is confusing, in my opinion, because it's unclear
whether it refers to the future version of removal, or to the current
running version.
Leave it as a setting with default true value.
Deprecations enabled or not, we only want to print deprecations for the
running major version, never for future versions.
Since deprecations are enabled by default.
@indirect
Copy link
Member

indirect commented Feb 22, 2019

Awesome! :)

@bundlerbot r+

ghost pushed a commit that referenced this issue Feb 22, 2019
6965: Turn on deprecations by default r=indirect a=deivid-rodriguez

### What was the end-user problem that led to this PR?

The problem was we've had deprecations in place for a long time, but we've never displayed them to users by default. 

### What was your diagnosis of the problem?

My diagnosis was that the current strategy doesn't work because:

* Printing deprecations as an optin feature almost never get enabled, so most users don't know about the stuff we'll be deprecating in the future.

* Printing deprecations in "deprecation releases" doesn't work well either, because it's unclear how long the deprecation release should last and thus how long we need to hold the final release (that will inhibit installation of the deprecation release).

### What is your fix for the problem, implemented in this PR?

My fix is to remove the concept of deprecation releases, and to add a feature flag for printing major deprecations that it's enabled by default.

As a extra related change, I also reworded the deprecation messages, because I find the current message "[DEPRECATED FOR 2.0] <Message about the deprecation>" a bit confusing because it's unclear what the version printed is referring to (deprecation horizon? current running version?), so I changed it to just "[DEPRECATED] <Message about the deprecation>".

### Why did you choose this fix out of the possible options?

I chose this fix because it (once released) finally makes it so that users will know about our deprecations.


Co-authored-by: David Rodríguez <deivid.rodriguez@riseup.net>
@ghost
Copy link

ghost commented Feb 22, 2019

Build succeeded

@ghost ghost merged commit fda9db6 into master Feb 22, 2019
3 checks passed
@ghost ghost deleted the deprecation_changes branch Feb 22, 2019
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants