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.

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 ultra verbose warning formatter. #305

Merged
merged 1 commit into from Nov 5, 2014

Conversation

troessner
Copy link
Owner

In regards to issue #302

@troessner troessner force-pushed the add-ultra-verbose-warning-formatter branch 3 times, most recently from 18c9b66 to 1881373 Compare November 2, 2014 22:13
@troessner troessner changed the title [WIP] Add ultra verbose warning formatter. Add ultra verbose warning formatter. Nov 2, 2014
@troessner
Copy link
Owner Author

@mvz ready for review.

2 remarks:

1.) We'd need to rename our wiki pages accordingly so that they match SmellWarning#subclass, but I don't see this as a problem.
2.) How about making reek more user friendly and make this default? Advanced reek users will have their special configuration anyway, why not make it easy for new users?

@mvz
Copy link
Collaborator

mvz commented Nov 3, 2014

1.) Not really a problem I think.
2.) Perhaps, but I'd like to be able to play around with this option a bit first. One option mentioned in #302 is to have explanatory text in the output itself, for example. Once it's fleshed out, we can decide on a default. Another thing is that we don't yet have a way to set default command line arguments, so making -U the default would become a bit annoying for non-beginners. Finally, I think changing the default output should be part of a major release, so we can perhaps consider this for release '2'.

@@ -88,6 +88,9 @@ def set_options
@parser.on('-V', '--no-quiet', '--verbose', 'Show headings for smell-free source files') do |_opt|
@strategy = Report::Strategy::Verbose
end
@parser.on('-U', '--ultra-verbose', 'Try to be as explanatory as possible') do |_opt|
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would drop the 'Try to' here.

@mvz
Copy link
Collaborator

mvz commented Nov 3, 2014

I think this PR is a good start for ultra-verbose mode. I'd like to continue this by adding some explanatory text in the output itself. What do you think?

@troessner troessner force-pushed the add-ultra-verbose-warning-formatter branch from 1881373 to a9cc0f3 Compare November 3, 2014 10:48
@troessner
Copy link
Owner Author

@mvz I addressed all your comments, thanks for the feedback!

Finally, I think changing the default output should be part of a major release, so we can perhaps consider this for release '2'.

Yes, makes sense.

I'd like to continue this by adding some explanatory text in the output itself. What do you think?

Please go ahead..:)

mvz added a commit that referenced this pull request Nov 5, 2014
…atter

Add ultra verbose warning formatter.
@mvz mvz merged commit 4ebaf12 into master Nov 5, 2014
@mvz mvz deleted the add-ultra-verbose-warning-formatter branch November 5, 2014 06:33
@troessner
Copy link
Owner Author

Actually I forgot that github generates all those links out of the wiki pages title.
So instead of adjusting the wiki page title we should rather adjust the link generation to be

https://github.com/troessner/reek/wiki/Irresponsible-Module

instead of

https://github.com/troessner/reek/wiki/IrresponsibleModule

This was referenced Nov 6, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants