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
Conversation
There was a problem hiding this 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
tools/lint/attribute.rb
Outdated
|
||
require "syntax_tree" | ||
|
||
module Visitor |
There was a problem hiding this comment.
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 visitor
s ended up duplicated under tools/lint
0cd8a0e
to
b3c1cf9
Compare
I opened some example PRs on my fork to show that the Edit: hmm, looks like putting it in the Gemfile may not work... |
61e1c9c
to
9953aa2
Compare
730cfc2
to
b2a66c4
Compare
@@ -0,0 +1,24 @@ | |||
name: Rail Inspector |
There was a problem hiding this comment.
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 🤔
e9e3ccf
to
3221e29
Compare
Co-authored-by: zzak <zzakscott@gmail.com>
b374de3
to
2961560
Compare
Alternative to #47813 to import the linter files directly to Rails source tree