ISSUE-1734 Add missing configuration warning #1741
Conversation
This PR adds a more descriptive error message when users add a detector without configuration. Before this change a NoMethodError exception was raised with an unclear error message. Now we catch this exception and provide a more helpful error message. A better future option might be add [dry-schema](https://dry-rb.org/gems/dry-schema/1.13/) See: troessner#1734
| @@ -80,6 +80,8 @@ def strings_to_regexes_for_detectors | |||
| to_regex item | |||
| end | |||
| end | |||
| rescue NoMethodError | |||
| warn_about_missing_configuration(detector) if detectors[detector].nil? | |||
There was a problem hiding this comment.
I think it's better to detect that detector[detector] is nil at the beginning of this block and handle that specific case there.
| @@ -101,10 +103,25 @@ def strings_to_regexes_for_directories | |||
| to_regex item | |||
| end | |||
| end | |||
| rescue NoMethodError | |||
| warn_about_missing_configuration(detector) if configuration.nil? | |||
There was a problem hiding this comment.
I think it's better to detect that configuration is nil at the beginning of this block and handle that specific case there.
| end | ||
| end | ||
| end | ||
| end | ||
|
|
||
| def warn_about_missing_configuration(detector) |
There was a problem hiding this comment.
The way error handling works in Reek is that we raise specific errors throughout the code and then do the warning in lib/reek/cli/application.rb.
We do this, among other reasons, because it allows us to test the error handling code without having warnings appear all over the place during the spec run.
In this case, the error should probably be a Reek::Errors::ConfigFileError. It will be handled here:
reek/lib/reek/cli/application.rb
Line 48 in b1fa495
| @@ -21,8 +21,10 @@ module DefaultDirective | |||
| # @return [self] | |||
| def add(detectors_configuration) | |||
| detectors_configuration.each do |name, configuration| | |||
| detector = key_to_smell_detector(name) | |||
| self[detector] = (self[detector] || {}).merge configuration | |||
| if configuration | |||
There was a problem hiding this comment.
Empty configs should be filtered out earlier on. In fact, if you move the handling of the error, Reek will not run with nil configurations so this condition will no longer be needed.
|
An alternative solution would be to move to |
|
Agree with all of @mvz comments so far :) |
|
Making this a WIP PR, waiting on #1749 |
This PR adds a more descriptive error message when users add a detector without configuration. Before this change a NoMethodError exception was raised with an unclear error message. Now we catch this exception and provide a more helpful error message.
A better future option might be add dry-schema.
The current yaml schema validator is outdated and does not seem to have good documentation.
See: #1734