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

update ActiveSupport::TestCase#assert_not to be ruby styleguide compliant #40247

Closed
wants to merge 1 commit into from
Closed

update ActiveSupport::TestCase#assert_not to be ruby styleguide compliant #40247

wants to merge 1 commit into from

Conversation

SampsonCrowley
Copy link
Contributor

@SampsonCrowley SampsonCrowley commented Sep 17, 2020

Summary

  • use refute value, not assert !value
  • add additional testing for "truthy" values

Other Information

The general ruby style guide says to prefer refute over assert !

The current implementation goes directly against that under the premise that "minitest might change refute" (see here)

  1. that is not a good reason to not use refute, in my opinion.
    • Minitest is not going to randomly change one of the two core methods of the framework on a whim.
  2. instead of going against the style guide on the extremely slim chance that Minitest completely rewrites their core methods I believe it's better to follow the style guide, and instead add additional testing to check that the values we want are properly accepted or rejected
    • this has the benefit of following the core ruby style guide
    • it also verifies that if a change is made in Minitest, our testing will notify us of that change

…pliant

* use refute value, not assert !value
* add additional testing for "truthy" value
@georgeclaghorn
Copy link
Contributor

Thanks for the PR, but (a) this is a cosmetic change, which we don’t accept and (b) we don’t follow the (unofficial) Ruby Style Guide.

@SampsonCrowley
Copy link
Contributor Author

(a) Sorry, didn't think about that.

As for (b) No, not officially, but the basic rules of the general Ruby guide are well known to all of the Ruby devs I know, and I think it's fair to say those rules that don't violate the rails guide can still have value for recognizability and implicit logic. Because refute not only already exists, but is the preferred way of the authors of the library, the existence of the assert_not method was a curious ocurance without an immediate explanation. It wasn't clear whether assert_not was an alias for or just equivalent to refute or if there was a definitive performance reason for using it or why.

@georgeclaghorn if I update the docs and/or testing guide page to include a link to the rails style guide or note that the method is equivalent to refute would that be acceptable?

@SampsonCrowley SampsonCrowley deleted the assert_not_styleguide_compliance branch September 22, 2020 18:12
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.

None yet

2 participants