Changes to creation of rule set #154
Conversation
| @@ -13,7 +13,11 @@ Rake::TestTask.new do |test| | |||
| test.verbose = true | |||
| end | |||
|
|
|||
| RuboCop::RakeTask.new | |||
| RuboCop::RakeTask.new do |t| | |||
| # allow you to run "$ rake rubocop -a" to autofix | |||
There was a problem hiding this comment.
prefer something like
RuboCop::RakeTask.new :fmt do |t|
t.options << '-A'
end
or a rubocop:reformat etc, but might refactor that later
There was a problem hiding this comment.
I wouldn't default to -A and it is unsafe.
RuboCop::RakeTask.new created three tasks.
$ rake -T
rake rubocop # Run RuboCop
rake rubocop:autocorrect # Autocorrect RuboCop offenses (only when it's safe)
rake rubocop:autocorrect_all # Autocorrect RuboCop offenses (safe and unsafe)
So I could use rake rubocop:autocorrect to format my code but. The way I work is that I make some changed to the code without thinking to much about style, then run specs and I ran the specs with rake (the default task) and in those cases it would stop at rubocop. In anouther project I added these two options so I could run rake -a. I could also change to run rake test.
So maybe I should just drop this change
| # +filename+ can be a string or uri pointing to the file or url location. | ||
| # +offset+ should be Range object representing the start and end byte locations where the rule was found in the file. | ||
| def add_rule!(*args, selectors: nil, block: nil, filename: nil, offset: nil, media_types: :all) # rubocop:disable Metrics/ParameterLists | ||
| if args.any? |
There was a problem hiding this comment.
ideally not support args at all without deprecated: true being set, but still a good step forward
There was a problem hiding this comment.
Maybe I'm not understanding what your suggesting.
It would be a breaking change it we now require deprecated: true to pass keyword arguments. If somebody are doing the change to add the that keyword argument why don't they change all to keywords arguments.
| when 4 | ||
| filename, offset, selectors, block = args | ||
| when 5 | ||
| filename, offset, selectors, block, specificity = args | ||
| else |
There was a problem hiding this comment.
why do we need to add this when we only supported selectors, block, specificity before ?
There was a problem hiding this comment.
because I removed OffsetAwareRuleSet. I was thinking we could have less conditionals if we merge those
|
rubocop failed |
Deprecation message look like: /Users/simon/Downloads/css_parser/test/test_css_parser_basic.rb:75: warning: [DEPRECATION] positional arguments are deprecated use keyword instead.
|
I think we can patch 1.x to have deprecation for changed to public API, and create update v2 branch without those. Probably a good idea to change to version 2 when we replace the css parser |
stoivo
force-pushed
the
st-refactor-creation-of-rule-set
branch
from
59f0fa5
to
219d43e
Compare
|
I'm not a fan of deprecations and the extra complexity they bring, but this looks most well though out to me so 👍 |
I am working on solving #128. To get to know the code I did some changed which I think makes it overall better.
I could try not to change anything else that what I absolutely have to but I think we will come better out of it if we make some more changes. Could you take a look at this changes and give me some pointer if you like this or not?