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

Enable Rails related Rubocop rules #9234

Merged
merged 13 commits into from May 11, 2022

Conversation

alecslupu
Copy link
Contributor

@alecslupu alecslupu commented May 5, 2022

🎩 What? Why?

Please describe your pull request.

πŸ“Œ Related Issues

Link your PR to an issue

Testing

Describe the best way to test or validate your PR.

πŸ“‹ Checklist

🚨 Please review the guidelines for contributing to this repository.

  • ❓ CONSIDER adding a unit test if your PR resolves an issue.
  • βœ”οΈ DO check open PR's to avoid duplicates.
  • βœ”οΈ DO keep pull requests small so they can be easily reviewed.
  • βœ”οΈ DO build locally before pushing.
  • βœ”οΈ DO make sure tests pass.
  • βœ”οΈ DO make sure any new changes are documented in docs/.
  • βœ”οΈ DO add and modify seeds if necessary.
  • βœ”οΈ DO add CHANGELOG upgrade notes if required.
  • βœ”οΈ DO add to GraphQL API if there are new public fields.
  • βœ”οΈ DO add link to MetaDecidim if it's a new feature.
  • ❌AVOID breaking the continuous integration build.
  • ❌AVOID making significant changes to the overall architecture.

πŸ“· Screenshots

Please add screenshots of the changes you're proposing
Description

β™₯️ Thank you!

@alecslupu alecslupu added dependencies Pull requests that update a dependency file or issues that talk about updating dependencies type: internal PRs that aren't necessary to add to the CHANGELOG for implementers labels May 5, 2022
@alecslupu alecslupu force-pushed the ale-fix-Rails-rubocops branch 2 times, most recently from 9a4cd75 to f79df48 Compare May 5, 2022 10:42
@alecslupu alecslupu changed the title Ale fix rails rubocops Enable rails related rubocops May 5, 2022
@alecslupu alecslupu changed the title Enable rails related rubocops Enable rails related Rubocop rules May 5, 2022
@alecslupu alecslupu marked this pull request as ready for review May 6, 2022 04:13
Copy link
Contributor

@ahukkanen ahukkanen left a comment

Choose a reason for hiding this comment

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

I just noticed one potential issue below but I also think it's outside of the scope for this PR.

I also noticed you left Rails/I18nLocaleTexts enabled, is it on purpose?

Rails/I18nLocaleTexts:
Enabled: false

The only violation from that seems to be here:

validates :voting_type, presence: true, inclusion: { in: Votings::Voting.voting_types, message: "%{value} is not a valid voting type" }

Can we fix that too?

@alecslupu
Copy link
Contributor Author

Can we fix that too?

Yes, it was left on purpose as that translation key may be tricky ( i do not have a working election test suite on my local ).
I would leave it at the moment the way it is, close the most of rubocop violations, and handle the 'hard' ones at the end.

@alecslupu
Copy link
Contributor Author

Can we fix that too?

Yes, it was left on purpose as that translation key may be tricky ( i do not have a working election test suite on my local ). I would leave it at the moment the way it is, close the most of rubocop violations, and handle the 'hard' ones at the end.

@ahukkanen , the requested violation has been fixed in 223f0cd

@alecslupu alecslupu requested a review from ahukkanen May 10, 2022 08:28
@alecslupu
Copy link
Contributor Author

@ahukkanen , can you have a look on this, so we can minimize the rubocop issues ? I am waiting for this to be merged, so I can unlock #9272

Copy link
Contributor

@ahukkanen ahukkanen left a comment

Choose a reason for hiding this comment

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

Thanks @alecslupu, it looks good now.

There's still the dependent: :destroy thing I mentioned but I think that's a wider problem in Decidim and I don't think it's very common case.

Could you please remove this comment from the disabled rules:

#Rails/I18nLocaleTexts:
# Enabled: false

After that it's good to go for me.

@alecslupu
Copy link
Contributor Author

Thanks @alecslupu, it looks good now.

There's still the dependent: :destroy thing I mentioned but I think that's a wider problem in Decidim and I don't think it's very common case.

Could you please remove this comment from the disabled rules:

#Rails/I18nLocaleTexts:
# Enabled: false

After that it's good to go for me.

The commented code removed in 463aff2
The dependent: :destroy, should be handled separately by module, but that may be a chore work to be done.

@ahukkanen
Copy link
Contributor

The dependent: :destroy, should be handled separately by module, but that may be a chore work to be done.

Yep, I created a separate issue #9291 to track that.

@ahukkanen ahukkanen changed the title Enable rails related Rubocop rules Enable Rails related Rubocop rules May 11, 2022
@ahukkanen ahukkanen merged commit 117263a into decidim:develop May 11, 2022
@alecslupu alecslupu deleted the ale-fix-Rails-rubocops branch May 12, 2022 05:20
andreslucena pushed a commit that referenced this pull request May 19, 2022
* Fix Rails/DuplicateScope

* Fix Rails/DurationArithmetic

* Fix Rails/CompactBlank

* Fix Rails/EagerEvaluationLogMessage

* Fix Rails/RedundantPresenceValidationOnBelongsTo

* Fix Rails/ExpandedDateRange

* Fix Rails/TransactionExitStatement

* Fixing Validators

* Fix Rails/HasManyOrHasOneDependent

* Clean up .rubocop-disabled.yml

* Clean up the rubocop-disabled file

* Fix Rails/I18nLocaleTexts

* Remove commented code
andreslucena pushed a commit that referenced this pull request May 19, 2022
* Fix Rails/DuplicateScope

* Fix Rails/DurationArithmetic

* Fix Rails/CompactBlank

* Fix Rails/EagerEvaluationLogMessage

* Fix Rails/RedundantPresenceValidationOnBelongsTo

* Fix Rails/ExpandedDateRange

* Fix Rails/TransactionExitStatement

* Fixing Validators

* Fix Rails/HasManyOrHasOneDependent

* Clean up .rubocop-disabled.yml

* Clean up the rubocop-disabled file

* Fix Rails/I18nLocaleTexts

* Remove commented code
@alecslupu alecslupu added this to the 0.27.0 milestone Jul 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file or issues that talk about updating dependencies type: internal PRs that aren't necessary to add to the CHANGELOG for implementers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants