-
Notifications
You must be signed in to change notification settings - Fork 47
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
Allow remaster in Naming/InclusiveLanguage #564
Allow remaster in Naming/InclusiveLanguage #564
Conversation
👋 @kpost, looks like you still need to sign the CLAhttps://cla.shopify.com/. Can you do so when you get a chance? (logs) |
@larouxn Done. However it seems I can't rerun the job. |
👋 @kpost, it looks like you're all good CLA wise. However, there are some failing tests. They look to be related to the config not being up to date. Should probably try a Finished in 0.269503s, 18.5526 runs/s, 22.2632 assertions/s.
1) Failure:
ConfigTest#test_config_is_unchanged [/home/runner/work/ruby-style-guide/ruby-style-guide/test/config_test.rb:34]:
Error: unexpected RuboCop configuration changes were detected.
- !ruby/regexp /\w*:\/\/\S+/
- !ruby/regexp /(?:blob|tree)\/master/
- !ruby/regexp /origin[ \/]master/
- mastercard
- webmaster
+ - remaster
Naming/MemoizedInstanceVariableName:
Description: Memoized method name should match memo instance variable name.
Enabled: false
VersionAdded: '0.53'
VersionChanged: '1.2'
If these changes are intentional, please update the config dump
by running `bundle exec rake config:dump`. |
rubocop.yml
Outdated
- 'webmaster' | ||
- 'remaster' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we just allow any unbroken word that contains "master"? Something like /\wmaster\w/
.
rubocop.yml
Outdated
- 'mastercard' | ||
- 'webmaster' | ||
- 'remaster' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would allow us to consolidate and cover future usage.
- 'mastercard' | |
- 'webmaster' | |
- 'remaster' | |
- !ruby/regexp '/(?<=[a-z])master|master(?=[a-z])/' # "master" substring within a longer word |
I don't think we want to restrict usage of words that include "master", just "master" itself.
Using [a-z]
ensures we still flag things like method_names_including_master
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, but I figured lots of words that include 'master' could still be considered not inclusive. Like 'mastering' or something? But I'm not really sure what is considered bad or good in this context anymore tbh.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like @sambostock's suggestions. Let's do it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kpost can you update the branch accordingly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely. Done. Apologies, didn't get around to it last couple days.
So I noticed there aren't any unit tests or something in place to test these changes in the default Rubocop config? Is that correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's correct. Most cops are simple enough that it makes sense to rely on the upstream tests in Rubocop.
However, #483 started to explore that idea for cops with configuration, given we'd seen some weird corrections for some rules, but it turns out it was due to Rubocop having a "blind spot" for hash entry value indentation when they appear on different lines than the keys (discussed here). We may circle back to it, if time permits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. It feels like it wouldn't be a bad idea though. At this point writing rubocop config almost feels like a DSL and testing the config more and more necessary. For now its fine though. Thanks for merging!
28156f6
to
bf0a169
Compare
Thank you @kpost! |
In the same spirit as 'Mastercard' and 'webmaster', 'remaster' (as in audiophonic or videographic remastering of older recordings etc) should be allowed as well.