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

Tco46 linting #80

Merged
merged 16 commits into from
Aug 13, 2024
Merged

Tco46 linting #80

merged 16 commits into from
Aug 13, 2024

Conversation

JPrevost
Copy link
Member

@JPrevost JPrevost commented Aug 12, 2024

Summary of changes (please refer to commit messages for full details)

This introduces a rubocop configuration and performs a series of auto corrections, creates a todo list we can come back to over time, and makes a few manual changes (demonstrating how we'll update the todo list as we address code that currently does not match our linting standard).

Developer

Ticket(s)

https://mitlibraries.atlassian.net/browse/TCO-46
https://mitlibraries.atlassian.net/browse/TCO-60

Accessibility

  • ANDI or Wave has been run in accordance to our guide and
    all issues introduced by these changes have been resolved or opened
    as new issues (link to those issues in the Pull Request details above)
  • There are no accessibility implications to this change

Documentation

  • Project documentation has been updated, and yard output previewed
  • No documentation changes are needed

ENV

  • All new ENV is documented in README.
  • All new ENV has been added to Heroku Pipeline, Staging and Prod.
  • ENV has not changed.

Stakeholders

  • Stakeholder approval has been confirmed
  • Stakeholder approval is not needed

Dependencies and migrations

YES dependencies are updated

NO migrations are included

Reviewer

Code

  • I have confirmed that the code works as intended.
  • Any CodeClimate issues have been fixed or confirmed as
    added technical debt.

Documentation

  • The commit message is clear and follows our guidelines
    (not just this pull request message).
  • The documentation has been updated or is unnecessary.
  • New dependencies are appropriate or there were no changes.

Testing

  • There are appropriate tests covering any new functionality.
  • No additional test coverage is required.

`bundle exec rubocop -a app/models/`
`bundle exec rubocop -A app/models/`
`bundle exec rubocop -a app/controllers/`
`bundle exec rubocop -A app/controllers/`
`bundle exec rubocop -a app/graphql/`
`bundle exec rubocop -A app/graphql/`

Note: reverted change to tacos_schema.rb that removed an example of how to do custom error handling that felt useful to keep in place even if it doesn't do anything yet.
`bundle exec rubocop -a test/`
`bundle exec rubocop -A test/`
`bundle exec rubocop -A lib/tasks/`
Note: removed changed to `suggested_resource.rake` as it caused a test failure
`bundle exec rubocop -A`

Notes:
auto cleanup on `app/dashboards/detector/suggested_resource_dashboard.rb` was incorrect as the initial code seemed a bit odd. I believe my manual fix is correct.

Removed autofix for tacos_schema.rb as it removed a good example of how to handle errors
We have a fair amount of not-autofixable code we have already accepted into the repository.
Rather than failing on the same code each time we run the linters, we can note which code is
known to be not following our standards and only fail the linters on changed code.

`bundle exec rubocop --auto-gen-config` is the way to do that.
Adds a few things the autogen config misses. Until we fix these, the autogen config will be
annoying as it will keep losing these exclusions.
@mitlib mitlib temporarily deployed to tacos-api-pipeline-pr-80 August 12, 2024 20:37 Inactive
Copy link
Contributor

@jazairi jazairi left a comment

Choose a reason for hiding this comment

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

Reviewed as suggested, focusing on the config files and Makefile rather than the corrections that Rubocop made. Confirmed, also, that the rubocop_todo file works as expected. Good idea to use that, btw; I'm not sure I was aware it was available.

:shipit:

@JPrevost JPrevost merged commit b8a272d into main Aug 13, 2024
2 checks passed
@JPrevost JPrevost deleted the tco46-linting branch August 13, 2024 13:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants