Add settings checks #41
Conversation
|
When config value is invalid: |
|
@gingermusketeer 👍 Looks good in principle. It would be good to see a benchmark as a speed increase would be great 😃 . I guess config versioning could be implemented also ref: #32. |
|
@mjago How do you normally go about benchmarking rufo? Would it make sense to have some tools for this in the codebase? |
lib/rufo/formatter/settings.rb
Outdated
| default = valid_options.first | ||
| value = options.fetch(name, default) | ||
| unless valid_options.include?(value) | ||
| STDERR.puts "invalid value for #{name}: #{value}. Valid values are: " \ |
There was a problem hiding this comment.
Inspect value like in
STDERR.puts "invalid value for #{name}: #{value.inspect}
lib/rufo/formatter/settings.rb
Outdated
| value = options.fetch(name, default) | ||
| unless valid_options.include?(value) | ||
| STDERR.puts "invalid value for #{name}: #{value}. Valid values are: " \ | ||
| "#{valid_options.join(', ')}" |
There was a problem hiding this comment.
Inspect valid options:
"#{valid_options.map(&:inspect).join(', ')}"Before: Valid values are: dynamic, one
After: Valid values are: :dynamic, :one
lib/rufo/formatter/settings.rb
Outdated
| def double_newline_inside_type(value) | ||
| @double_newline_inside_type = no_dynamic("double_newline_inside_type", value) | ||
| end | ||
| attr_accessor *OPTIONS.keys |
There was a problem hiding this comment.
Do we need real setters and getters?
Currently, these are just instance variables.
lib/rufo/formatter/settings.rb
Outdated
| "#{valid_options.join(', ')}" | ||
| value = default | ||
| end | ||
| self.send("#{name}=", value) |
There was a problem hiding this comment.
This should change to instance_variable_set if we want to avoid attr_accessor above.
lib/rufo/command.rb
Outdated
| end | ||
|
|
||
| formatter.format | ||
| formatter.result | ||
| end | ||
|
|
||
| def parse_dot_file(file_contents) | ||
| file_contents.lines |
There was a problem hiding this comment.
How about using each_with_object and split config line in exact 2 parts?
file_contents.lines
.map { |s| s.strip.split(/\s+/, 2) }
.each_with_object({}) do |(name, value), acc|
if value.start_with?(':')
value = value[1..-1].to_sym
elsif value == 'true'
value = true
elsif value == 'false'
value = false
else
STDERR.puts "Unknown config value=#{value.inspect} for #{name.inspect}"
next
end
acc[name.to_sym] = value
end
|
@gingermusketeer re. benchmarking I usually use a project with a large number of ruby files as the test to magnify the parsing speed value. Then in git I reset any changes made to the project by Rufo to reset the start position. One such large project is ruby/spec (5000 or so files). |
|
I did a little benchmarking on this looks to be a slight improvement of 2-4 seconds for a set of 10 runs against the ruby/spec code base. |
lib/rufo/formatter/settings.rb
Outdated
| "values are: #{valid_options.map(&:inspect).join(', ')}" | ||
| value = default | ||
| end | ||
| self.instance_variable_set("@#{name}", value) |
There was a problem hiding this comment.
Relying on instance variables seems error prone to me, leaves us open to typoing the setting entirely in the formatter and not realising it. What about adding
attr_accessor *OPTIONS.keys... to the class, and replacing this with
public_send(name, value)
and then referring to these settings by their methods rather than instance variables in the formatter?
There was a problem hiding this comment.
Side note, this isn't you, but having rufo/formatter/settings.rb reopen an existing class and add methods to it had me very confused. How about this is instead a Rufo::Settings module that is mixed into Rufo::Formatter explicitly?
Why: So that accessing non existent settings raises a no method error.
|
@bessey Thanks for the feedback. I think that makes sense so I have gone a head and changed it. Your first bit of feedback is counter to something that was touched on in some earlier feedback. I think you do have a good point about relying on instance variables and I have changed the formatter code so that we get the benefit. @splattael Be good to get your input on the last few commits. |
|
Oops, sorry to have introduced conflict. I tend to avoid @instance_vars since a typo can creep in silently, but admittedly in a code base as well tested as rufo, that may be less of a concern than it usually is to me. Thank you for making the changes nonetheless! |
|
Going to merge this. Let me know if anything else needs addressing and I will do that in a subsequent PR. |
Let me know what you think of this fix for #32.
Note: This removes the eval of the dot file in favor of manually parsing the config file.
Remaining changes: