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

Import rails-bin linter to rails internal tools lib #48216

Merged
merged 3 commits into from Sep 13, 2023

Conversation

zzak
Copy link
Member

@zzak zzak commented May 13, 2023

Alternative to #47813 to import the linter files directly to Rails source tree

$ zzak@mbp16 rails % ruby -v
ruby 3.2.2 (2023-03-30 revision e51014f9c0) [arm64-darwin22]



$ zzak@mbp16 rails % bundle exec tools/check-changelogs .
.............

13 changelogs inspected, 0 offenses detected



$ zzak@mbp16 rails % bundle exec tools/check-config-docs .



$ zzak@mbp16 rails % bundle exec tools/check-links .
https://edgeapi.rubyonrails.org/classes/ActiveSupport/MessageEncryptor.html#method-i-encrypt_and_sign

Copy link
Member

@skipkayhil skipkayhil left a comment

Choose a reason for hiding this comment

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

I think I'd prefer importing rail_inspector just because the scripts were all combined into the single Thor entrypoint. If you think check-links is valuable I can add that to rail_inspector as that is currently missing. I think regen-dummy could also be worth importing, but it seems less important due to Rafael's comment: #47259 (comment)

Something else that probably needs to be done is better namespacing of the modules. I'm realizing now that these are all in the global namespace so it may be better to nest them when running in the context of a larger Gemfile. (I've opened an issue for that here: skipkayhil/rail_inspector#2)

Edit: I'm also noticing that this fails Rails' rubocop. Opened an issue for that too: skipkayhil/rail_inspector#3

Ah something else: I would be disappointed if we lose the changelog tests. They've been really useful in ensuring changes don't regress. With that being said, I don't know what running those tests in CI here would look like. I'm assuming it would be a Github Action and they should also probably only run if these files are being changed


require "syntax_tree"

module Visitor
Copy link
Member

Choose a reason for hiding this comment

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

I think all of the visitors ended up duplicated under tools/lint

@skipkayhil
Copy link
Member

skipkayhil commented Jun 7, 2023

I opened some example PRs on my fork to show that the rail_inspector.yml workflow will only run if the folder is touched (and in addition, since the whole workflow is skipped it won't show up in the job list either):
skipkayhil#3 <- no change, no rail_inspector workflow
skipkayhil#4 <- folder has change, rail_inspector workflow is run

Edit: hmm, looks like putting it in the Gemfile may not work...

Gemfile Show resolved Hide resolved
tools/rail_inspector/README.md Outdated Show resolved Hide resolved
tools/rail_inspector/README.md Outdated Show resolved Hide resolved
@skipkayhil skipkayhil force-pushed the import-rails-bin-linter branch 2 times, most recently from 730cfc2 to b2a66c4 Compare June 11, 2023 23:12
@@ -0,0 +1,24 @@
name: Rail Inspector
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm wondering if we need to add this workflow if we're going to move this to buildkite soon anyways 🤔

@rafaelfranca rafaelfranca force-pushed the import-rails-bin-linter branch 2 times, most recently from e9e3ccf to 3221e29 Compare September 13, 2023 01:18
@rafaelfranca rafaelfranca merged commit 9150a98 into rails:main Sep 13, 2023
4 of 5 checks passed
@zzak zzak deleted the import-rails-bin-linter branch September 13, 2023 02:03
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.

None yet

3 participants