-
Notifications
You must be signed in to change notification settings - Fork 122
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
Support RuboCop 1.38 #288
Support RuboCop 1.38 #288
Conversation
This version of RuboCop needs registry and config attributes to be set on the ProcessedSource object. These attributes do not exist in earlier versions of RuboCop.
Perhaps another Gemfile needs to be added in |
if ::RuboCop::Version::STRING.to_f >= 1.38 | ||
registry = RuboCop::Cop::Registry.global | ||
source.registry = registry | ||
source.config = @rubocop_config | ||
end |
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.
Can we do the check on feature rather than version?
if ::RuboCop::Version::STRING.to_f >= 1.38 | |
registry = RuboCop::Cop::Registry.global | |
source.registry = registry | |
source.config = @rubocop_config | |
end | |
source.registry = RuboCop::Cop::Registry.global if source.respond_to?(:registry=) | |
source.config = @rubocop_config if source.respond_to?(:config=) |
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.
Well, the two things go together so it doesn't seem sensible to have two conditions.
Also, there is already a version check in that file: https://github.com/Shopify/erb-lint/blob/9b7dec10f51cd5e505347aded25d4d7e1aec94c4/lib/erb_lint/linters/rubocop.rb#L39, so it seems consistent to handle this the same way.
Finally, if ever in the distant future erb-lint supports only RuboCop 1.38 and above, it will be easier to see that the condition can be removed.
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.
Fair enough
Thanks, @etiennebarrie! |
This fixes #286.