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

Rollback gem deprecate #3530

Merged
merged 2 commits into from Apr 19, 2020
Merged

Rollback gem deprecate #3530

merged 2 commits into from Apr 19, 2020

Conversation

bronzdoc
Copy link
Member

Description:

closes #3512

This PR does a few things:

  • Restore old depreciation method
  • Deprecates old deprecation method
  • Rename new version horizon deprecation methods

I will abide by the code of conduct.

@simi
Copy link
Member

simi commented Apr 19, 2020

I think this is good enough for now. We can take a look at internal deprecation improvements later.

@bronzdoc bronzdoc merged commit 8e0c039 into master Apr 19, 2020
@bronzdoc bronzdoc deleted the rollback_gem_deprecate branch April 19, 2020 20:10
repl == :none ? " with no replacement" : "; use #{repl} instead",
". It will be removed on or after %4d-%02d-01." % [year, month],
"\n#{target}#{name} called from #{Gem.location_of_caller.join(":")}",
]
Copy link
Member

Choose a reason for hiding this comment

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

I believe we are introducing here the same kind of issue we're trying to solve in #3506: we're showing a deprecation message only meant for gem authors to all users. And we're showing this deprecation message about the deprecation system right next to the deprecation of a method that end users do care about.

I feel we might cause brain damage to our users 😆.

So, I believe we should revert the deprecation of Gem::Deprecate for now until we figure out good strategies to be smart about whether we are in an end user context or in a gem development context, and apply them to this particular case.

Copy link
Member

Choose a reason for hiding this comment

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

I have missed that. I agree on removing this deprecation warning for now.

Copy link
Member

Choose a reason for hiding this comment

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

FWIW, I placed this comment at the wrong place, my comments refers to the line below this one, just to clarify.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure if there's other way to do this without end users seeing the deprecation warning. 🤔

This is not something that we could check at build time...

Copy link
Member

Choose a reason for hiding this comment

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

I think we can try to approximate it. A first idea that came to my mind was whether there's a ".gemspec" file at the CWD or up in the directory hierarchy.

Copy link
Member

Choose a reason for hiding this comment

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

Can we just remove it for now (should I provide PR?) and discuss this as a follow-up in new issue?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, yes, that's exactly what I was suggesting.

Copy link
Member

Choose a reason for hiding this comment

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

##
# Simple deprecation method that deprecates +name+ by wrapping it up
# in a dummy method. It warns on each call to the dummy method
# telling the user of +repl+ (unless +repl+ is :none) and the
# Rubygems version that it is planned to go away.

def deprecate(name, replacement=:none)
def rubygems_deprecate(name, replacement=:none)
Copy link
Member

Choose a reason for hiding this comment

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

I think the new name conveys it quite well, but we should make it super clear that this is an internal module, not a public one for everyone to use. Not sure which strategies are tipically used for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Restore Gem::Deprecate to the way it worked in the previous release
4 participants