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

AO3-6691 Custom cops for house rules #4763

Merged
merged 12 commits into from
Apr 2, 2024

Conversation

brianjaustin
Copy link
Member

Issue

https://otwarchive.atlassian.net/browse/AO3-6691

Purpose

Adds custom Rubocop rules to automatically flag some things we have to do manually in PR review. For example, using departure for large tables.

@sarken sarken added Priority: Low Scope: Tests Only Only changes automated tests or test configuration labels Mar 5, 2024
Copy link
Contributor

@Bilka2 Bilka2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't look at the code itself much, my focus was more on the resulting messages and docs of the cops.

rubocop/cop/i18n/deprecated_helper.rb Outdated Show resolved Hide resolved
rubocop/cop/i18n/deprecated_translation_key.rb Outdated Show resolved Hide resolved
rubocop/cop/migration/large_table_schema_update.rb Outdated Show resolved Hide resolved
rubocop/cop/i18n/static_translation_definition.rb Outdated Show resolved Hide resolved
rubocop/cop/i18n/static_translation_definition.rb Outdated Show resolved Hide resolved
rubocop/cop/i18n/static_translation_definition.rb Outdated Show resolved Hide resolved
@Bilka2
Copy link
Contributor

Bilka2 commented Mar 28, 2024

Is it possible to check the arguments of the method call? In that case it would be nice to have a cop that checks for calls to t that include the default option. For example, t('.email_address', :default => "Email address") should have the translation in the .yml file and be just t(".email_address").

Comment on lines +7 to +8
# instead of Regex. Note: this may not always be possible, and this
# cop is not smart enough to detect those cases.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This means that the cop will have false failures in those cases, leading to false positive failing tests on PRs. That's not great. Perhaps that would be helped if the offense could be considered a low severity. Though I don't know if that will keep the test from failing while still showing up on GitHub, I'd have to investigate how reviewdog handles it.

But did find that add_offense indeed supports a severity level.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's what it looks like with severity: :convention https://github.com/brianjaustin/ao3-sandbox/pull/9/files

The action is still marked as failed, but at least the annotation looks a little bit less angry (the higher severities are either yellow or red with an ❌)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As tested here that unfortunately doesn't apply for commits from forks where reviewdog degrades to annotations. Oh well, was worth the try.

Copy link
Contributor

@Bilka2 Bilka2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the cops look good and I had a quick look over the code and tests as well, which looks fine (though I didn't look too deep).

@sarken sarken merged commit cedf5b6 into otwcode:master Apr 2, 2024
26 checks passed
@brianjaustin brianjaustin deleted the rubocop-custom-cops branch April 3, 2024 01:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: Low Reviewed: Ready to Merge Scope: Tests Only Only changes automated tests or test configuration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants