Skip to content
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.

Sign up for GitHub

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

Jump to bottom

ISSUE-1734 Add missing configuration warning #1741

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

ISSUE-1734 Add missing configuration warning #1741

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions lib/reek/configuration/configuration_converter.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's better to detect that detector[detector] is nil at the beginning of this block and handle that specific case there.

end
end
end
Expand All @@ -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?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

rescue Errors::ConfigFileError => error

msg = <<~ERROR
###############################

Please review the configuration file (e.g. .reek.yml in your project directory).
It looks like the configuration for #{detector} is missing.
You can find the configuration options documentation over here: https://github.com/troessner/reek#configuration-options.

###############################
ERROR
warn msg
end
end
end
end
6 changes: 4 additions & 2 deletions lib/reek/configuration/default_directive.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

detector = key_to_smell_detector(name)
self[detector] = (self[detector] || {}).merge configuration
end
end
self
end
Expand Down
Loading