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

deprecate any_instance_of family of methods #1423

Open
HoneyryderChuck opened this issue May 21, 2021 · 6 comments
Open

deprecate any_instance_of family of methods #1423

HoneyryderChuck opened this issue May 21, 2021 · 6 comments

Comments

@HoneyryderChuck
Copy link

deprecate any_instance_of family of methods

rubocop-rspec already discourages the use of allow/expect_any_instance_of, and a workaround is provided.

Also, from the rspec documentation around these methods, it's argued that using it is a "design smell", it's a "complicated feature", and "no one in the core team uses it".

In light of all that, couldn't the rspec team just deprecate the methods, and log the rubocop workaround? I'm saying that because it's hard for me to justify not using rspec features based on rubocop feedback (sometimes rubocop suggestions are just "opinionated"), and given all the described difficulties and the existence of an alternative solution, maybe the rspec team could send that clear message themselves via deprecation.

@pirj
Copy link
Member

pirj commented May 21, 2021

No objections to deprecating it. However, its removal should happen with a major version release.

A viable option would be to extract it to a gem, add it as a depency for rspec-mocks, and drop this dependency in the next major version. It might be difficult though.

With an upcoming release of RSpec 4 I believe it's too late to deprecate any_instance. We can do it once RSpec 4 is released.
It's not a dealbreaker to deprecate it now, though. History shows that some features were deprecated in RSpec 2, but not removed in RSpec 3. I don't say it's a good approach, but we can deprecate any_instance now in RSpec 3, and remove it from RSpec 5 (which will probably be released in 4-5 years from now).

Would you like to hack on extracting any_instance, or add deprecation warnings?
I'd expect us not to flood the log with warnings if the user has explicitly required the extracted gem. That probably means they are going to stick with it in the future, and there's no point of littering the output.

@HoneyryderChuck
Copy link
Author

Totally fine with the points you raised. Just feel that, overall, the decision of telling users not to use a non-recommended feature should ultimately come from the tool, rather than the linter.

Would you like to hack on extracting any_instance,...

No, sorry. I use rspec only at work. And I wouldn't keep using the feature even if it were via an optional dependency.

@pirj
Copy link
Member

pirj commented May 24, 2021

I use rspec only at work.

We all do. And companies rarely make time for their employees to contribute to open source.
Some brave hearts, though, pay back to projects they use in their daily jobs by sacrificing their spare time.

@HoneyryderChuck
Copy link
Author

Sorry, maybe I expressed myself incorrectly.

I don't want to extract the any_instance thing, as that would require me to depend on it, advocate for its continued use, and be prepared to maintain it further. And I'd rather stop using it, as the rspec team suggests.

Any further work would require me to need something or buying in to the project. Which I would do, and I've gladly done in the past for rspec-related projects. I think that, in this case, the rspec team "needs" to deprecate this API more than I do (I've seen "any_instance" mentioned in the issues page a lot), hence the suggestion. Feel free to close the ticket if you think otherwise.

I agree with your whole assessment "if you use it, be prepared to contribute and pay for it", I also maintain other OSS projects, so I can relate.

@pirj
Copy link
Member

pirj commented May 24, 2021

Not necessarily you'll have to commit to maintaining something you'll never use yourself.

@JonRowe What is the plan for the should syntax that will be removed from RSpec 4? Do we plan to extract it ourselves, or do we plan to shoulder this task to the first who complains about its removal?

@JonRowe
Copy link
Member

JonRowe commented May 24, 2021

Personally, I feel there is a difference between "we don't recommend" and "don't use"; Deprecation is the latter of those two, it's don't use as this will be removed soon. Until we have a plan to extract or otherwise remove it from the main rspec gems, we can't really deprecate it.

We as a team don't recommend it's use, but as far as we know it's still wildly used in Rails codebases which makes it painful for it to be officially deprecated without a plan to support it at least as an external gem.

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

No branches or pull requests

3 participants