-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Add explicit error when a section is empty #1784
Add explicit error when a section is empty #1784
Conversation
5ed8f57
to
17cc2c1
Compare
Looks good to me. @jonas054 what do you think? |
return unless @hash.key?(name) && @hash[name].nil? | ||
fail ValidationError, | ||
"empty section #{name} found " \ | ||
"in #{loaded_path || self}" |
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.
It’s not immediately obvious (to me, at least) what loaded_path || self
would return. Maybe extract it to a variable with a descriptive name?
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.
Hi,
That expression is used at least 2 other times in that file.
The way I see things, I could factorize it in a method called loaded_path_or_self, but I'm not sure it will add something to the code comprehension.
@bbatsov, what's your input on this ?
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.
Ah, I didn’t see that the expression was used already. Surely re-using it should be ok for this PR.
👍 Looks good! I agree that |
Thanks for your feedbacks, guys. |
11e69e2
to
bbd4905
Compare
it 'raises validation error' do | ||
expect { configuration.validate } | ||
.to raise_error(described_class::ValidationError, | ||
%r{^empty section Metrics\/LineLength}) |
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.
Why is / escaped?
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.
My mistake. Thanks for noticing.
bbd4905
to
b95689a
Compare
Please, rebase on top of the current |
b95689a
to
827d0c8
Compare
Done. |
👍 |
Add explicit error when a section is empty
Should this fix have worked it's way into v0.32.0? I'm getting the
When I remove the
Do you think this error is related to the bug this P.R. is fixing? |
Raises a ValidationError "empty section #{name} found in #{loaded_path || self}" instead of 'undefined method `each_key' for nil:NilClass' when the configuration file contains an empty section.
closes issue #1782