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

Allow remaster in Naming/InclusiveLanguage #564

Merged
merged 2 commits into from Aug 31, 2023

Conversation

kpost
Copy link
Contributor

@kpost kpost commented Aug 9, 2023

In the same spirit as 'Mastercard' and 'webmaster', 'remaster' (as in audiophonic or videographic remastering of older recordings etc) should be allowed as well.

@kpost kpost requested a review from a team as a code owner August 9, 2023 07:46
@github-actions github-actions bot added cla-needed config change Changes the Rubocop config by enabling, disabling, or reconfiguring one or many cops and removed cla-needed labels Aug 9, 2023
@larouxn
Copy link
Contributor

larouxn commented Aug 9, 2023

👋 @kpost, looks like you still need to sign the CLAhttps://cla.shopify.com/. Can you do so when you get a chance? (logs)

@kpost
Copy link
Contributor Author

kpost commented Aug 10, 2023

@larouxn Done. However it seems I can't rerun the job.

@larouxn
Copy link
Contributor

larouxn commented Aug 10, 2023

👋 @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 bundle exec rake config:dump. (CI logs)

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
Comment on lines 385 to 386
- 'webmaster'
- 'remaster'
Copy link
Contributor

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
Comment on lines 384 to 386
- 'mastercard'
- 'webmaster'
- 'remaster'
Copy link
Contributor

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.

Suggested change
- '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.

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor

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?

Copy link
Contributor Author

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?

Copy link
Contributor

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.

Copy link
Contributor Author

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!

@kpost kpost force-pushed the allow_remaster_inclusive_language branch from 28156f6 to bf0a169 Compare August 11, 2023 07:02
@sambostock sambostock merged commit 183b249 into Shopify:main Aug 31, 2023
11 checks passed
@sambostock
Copy link
Contributor

Thank you @kpost!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
config change Changes the Rubocop config by enabling, disabling, or reconfiguring one or many cops
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants