Version horizon deprecations

[bronzdoc]

Description:

Closes #3013

This PR makes deprecate and deprecate_command accept a date horizon and instead of a date,

---

I will abide by the code of conduct.

[deivid-rodriguez]

@bronzdoc Loving this! This is WIP, right? I mean, next steps would be to change the calls to use the new implemenation of deprecate and deprecate_command.

Also, can we default to set removal date for the next major is no explicit version is passed? Actually, we might want to remove the parameter altogether because I don't think it makes sense to deprecate something two major versions in advance?

[bronzdoc]

@deivid-rodriguez

might want to remove the parameter altogether because I don't think it makes sense to deprecate something two major versions in advance?

Good point, makes sense.

next steps would be to change the calls to use the new implemenation of deprecate and deprecate_command.

Yeah, my idea to implement it like this was that we might want to discuss when those methods were going to be deprecated(which version), but if we are going to set the deprecation to the next major always I think I should just get rid of the "date" methods and use the new ones.

What you think?

[deivid-rodriguez]

Yeah, I think removal on the next major is the safest thing to do?

[bronzdoc]

@deivid-rodriguez so if we set the default to be always the next major(calculated with the current major) without passing a major version explicitly, we might run into the same problem we have today. If we deprecate a method/command and we forget to deprecate it in the next major release we would be stuck with it for the next release.

So:
A: We pass the version to deprecate it explicitly and check that the version that we pass is not two majors ahead

B: We calculate it to always be the next major but we will have to build tooling to check for deprecations before a major release

C:?

[deivid-rodriguez]

Indeed!

I was actually going to propose B) as a follow up for this PR.

Isn't A) subject to the same problem? I we do deprecate :foo, :bar, version: 4 (or whatever signature we choose), aren't we subject to releasing rubygems 4 and forgetting to remove the code, and users getting messages about "foo will be deprecated in rubygems 4" when they're already using rubygems 4? I think we'll need to build tooling anyways?

[deivid-rodriguez]

Actually, I built some tooling to check the current date based deprecations:

rubygems/Rakefile

Lines 89 to 100 in 2bbcdcd

	task :prerelease => %w[clobber test bundler:build_metadata check_deprecations]
	task :postrelease => %w[bundler:build_metadata:clean upload guides:publish blog:publish]
	desc "Check for deprecated methods with expired deprecation horizon"
	task :check_deprecations do
	if v.segments[1] == 0 && v.segments[2] == 0
	sh("util/rubocop -r ./util/cops/deprecations --only Rubygems/Deprecations")
	else
	puts "Skipping deprecation checks since not releasing a major version."
	end
	end

But I set it to only be activated when releasing a major version, so it's never actually been run yet 😅.

So we need to adapt to custom rubocop cop to the new deprecations. Since we're killing the date based deprecations, I think it actually makes sense to adapt that code inside this PR.

Thoughts?

[bronzdoc]

@deivid-rodriguez

Isn't A) subject to the same problem? I we do deprecate :foo, :bar, version: 4

True, I think I was thinking on checking something like :version >= current_rubygems_major inside deprecate and do just return in the method override instead of alerting but i guess we don't get rid of the method and it will just make things more confusing lol

Thoughts?

Sounds good, we can adapt that to the new version horizon deprecation. Is there a benefit in making it a rubocop custom cop instead of a rake task?

[deivid-rodriguez]

It's actually a rake task implemented under the hood as a custom cop. When I implemented it, using rubocop looked more neat to me than "regexp'ing the source code", that why I used a custom cop. Since now it's already there, I think it shouldn't be hard to adapt it. The method name will be the same, only the argument checking logic will change.

[bronzdoc]

Errors are not related, seems like the bundler container can't connect to azure.archive.ubuntu.com to
install graphviz 😬

[deivid-rodriguez]

Nice @bronzdoc, it looks good to me now!

After a second though I'm not sure we even need the rubocop check. What if upon release, we decide that we don't want to remove yet one or two deprecations and stick with them for one more major cycle? In that case, the solution would be to leave the deprecations there, so that they also warn users after the release. So, there would be now way to get the rubocop check green and make the release in the case 😅.

We definitely need to add "Remove pending deprecations" to our release playbook, in the case of releasing a major, but maybe this automated check is unnecessary and might get in the middle.

In any case, this is a strict improvement over what we have now, so I'm happy to merge this and discuss the removal of the rubocop check separately.

[deivid-rodriguez]

I reviewed the changes to the rubocp cop, although as I explained, we might want to get rid of it. Something else we need to add if we keep it is to also detect deprecate_command calls.

[deivid-rodriguez]

Bad review, I wanted to request changes to the documentation of the cop 😅

[bronzdoc]

@deivid-rodriguez sounds good, lets keep it and decide if we want to remove it in the future.

Meanwhile I added a check for deprecate_command to the cop. Thanks for the review and let me know if I missed something 🙂

[deivid-rodriguez]

Two super minor comments, but other than that looks great to me!

[bronzdoc]

Thanks for the Review @deivid-rodriguez I addressed your changes, going to merge this 🚀

[deivid-rodriguez]

Thanks @bronzdoc!

[simi]

ℹ️ I have seen this deprecation mechanismus used in wild by gems extending Gem::Deprecate and those libs are broken now. For example appsignal/appsignal-ruby#599.

Since that method is not nodoc, would it be possible to get back original signature and postpone this change to next major version? Or do rubygems not care about this problem and should that be used in libs one by one for now? Is there any way to prevent extending of this outside of rubygems itself than?

[deivid-rodriguez]

Hi @simi, thanks for noticing this!

I guess we should keep the signature and deprecate the date and month parameters, but... deprecating a deprecation mechanism... makes my head ache 🤣.

Not sure, let's think about it a bit more. 🤔

[simi]

🤔 Alternatively we can keep the old deprecate method and rename the current one to deprecate_soon or something similar and use it where needed.

[deivid-rodriguez]

Yeah, I was thinking of something like that too. But it seems weird to give our new preferred deprecation mechanism an artificial name.

Something else I'm noticing is that, we've turned an (apparently public, not sure if that was ever intended) mechanism for deprecating generic methods into something rubygems specific, because the message now mentions rubygems. So that's another compatibility problem on top of that.

[deivid-rodriguez]

There's an accepted feature request in ruby-core to make this builtin, by the way: https://bugs.ruby-lang.org/issues/16018.

Currently I tend to think we should deprecate passing date and month, and explicitly pass a version. I believe that was @bronzdoc's initial implementation, and I told him that an explicit version parameter was not necessary. However, if we want to keep this method generic, it sounds like the easiest way.

I'll think of it a bit more.

[simi]

Yeah, I was thinking of something like that too. But it seems weird to give our new preferred deprecation mechanism an artificial name.

Right name is the key here. Maybe rubygems_deprecate can work better.

Something else I'm noticing is that, we've turned an (apparently public, not sure if that was ever intended) mechanism for deprecating generic methods into something rubygems specific, because the message now mentions rubygems.

The question in here is if rubygems is going to accept this fact and continue supporting Gem::Deprecation as is now (now = latest release). IMHO that would be the best to not get tons of angry library maintainers :).

There's an accepted feature request in ruby-core to make this builtin, by the way: https://bugs.ruby-lang.org/issues/16018.

It is not even developed and would take a long time unless it will be publicly used.

🤔 FYI I have spotted the same wild usage of rubygems tar reader/writer as well.

[deivid-rodriguez]

Regarding the ruby-core feature request, yeah, I only pointed to it as a reference, but it's not something useful at the moment.

Anyways, I definitely appreciate that you spotted this, this early, and I think we should definitely restore support for this at least for the next minor, since I'm sure it is indeed widely used.

Currently I tend to think we should deprecate passing date and month, and explicitly pass a version.

Actually, deprecations are too tricky. We probably should do this and instead restore the original implementation for now and add our new private mechanism for deprecating stuff ourselves, as you suggested.

[simi]

Regarding the ruby-core feature request, yeah, I only pointed to it as a reference, but it's not something useful at the moment.

Yeah, thanks for that. I had no idea about that.