Improve reek's default messages

[troessner]

I believe our default messages are bad since they are catered to people who kind of know what to do anyway.
I believe right now we are deterring reek beginners - we should make Reek so beginner-friendly that my grandmother would fully understand what our error messages mean without special switch by default. OR, and this is what I'm actually suggesting, link to our helpful doc pages right away because I believe that if you have a concise message with a link at the end, most people will click this.

E.g. I'd change this:

[5, 5]:ShoppingCart#gross_price refers to item more than self (maybe move it to another class?) (FeatureEnvy)

to:

[Line 5]: ShoppingCart#gross_price smells of FeatureEnvy

I talked to quite some Reek users recently and believe it or not, almost NOBODY had even realized that we had very helpful doc pages which not only explain the problem, but also often suggest a solution.

WDYT?

[chastell]

my grandmother would fully understand

<mild-cringe />

[Line 5]: ShoppingCart#gross_price smells of [FeatureEnvy](https://github.com/troessner/reek/blob/master/docs/Feature-Envy.md)

Oh yes please! I’m not sure about the Line in there (will it be Lines for multiple cases?), but a huge 👍 on the change. (Also the space after the colon: much appreciated!)

[troessner]

(will it be Lines for multiple cases?)

Yes!

[troessner]

@chastell I'd like to finish the UnusedPrivateMethod smell first, would you like to tackle this?

[chastell]

Sure!

• This would be Reek 4, right?
• In non-hypertext formats, should the URLs be just printed after the smell name?
• If we’re adding Line/Lines (and trading off pareability for readability), how about dropping the squeare brackets around the line numbers?

[troessner]

This would be Reek 4, right?

Nooooooooooooes. Let's NOT do this. While I an understand the reasoning, considering the smell warning messages themselves would kill all flexibility.
Think about it. Every small change, even a typo fix, would require a major version change.
People relying on our API shouldn't develop against our messages anyway, since those should be considered volatile.

In non-hypertext formats, should the URLs be just printed after the smell name?

Sounds good!

(and trading off pareability for readability)

What in the holy smurf is "pareability"?

how about dropping the squeare brackets around the line numbers?

👍

[chastell]

This would be Reek 4, right?

Nooooooooooooes. Let's NOT do this. While I an understand the reasoning, considering the smell warning messages themselves would kill all flexibility.

I wouldn’t consider the smell warning messages changes API-breaking (full agreement here), but I do consider the line number format change API-breaking. While I think people should use JSON or XML formats if they want to parse it, parsing the text output in shell scripts seems like a quite common (and even defendable) use case.

Example: I want to have an ASCII bar chart of how many smells there are of each type; the super-quick way to do this is to just pipe reek text output through a shell script that would (a) count the commas between the first pair of square brackets and add 1, (b) grep out the CamelCased smell name from the rest of the line and (c) tally the results.

I think in Reek 3 we should stick to the […comma-separated line numbers…]:…text with a CamelCased smell name… format – so for Reek 3 I would switch to [1,2]:BadSmell https://… and for Reek 4 upgrade it to Lines 1, 2: BadSmell https://…. WDYT?

What in the holy smurf is "pareability"?

Argh, apologies: parsability, like in the shell example above. :)

[chastell]

Also, I think it’s beneficial to add the relevant metadata – so rather than switch from ShoppingCart#gross_price refers to item more than self (maybe move it to another class?) (FeatureEnvy) to ShoppingCart#gross_price smells of FeatureEnvy I’d keep the reference to item somewhere. Similarly, in Dirty#awful has 4 parameters (LongParameterList) I’d keep the 4, and in Dirty#awful has boolean parameter 'log' (BooleanParameter) I’d keep the log. Thoughts?

[troessner]

I wouldn’t consider the smell warning messages changes API-breaking (full agreement here), but I do consider the line number format change API-breaking. While I think people should use JSON or XML formats if they want to parse it, parsing the text output in shell scripts seems like a quite common (and even defendable) use case.

Good point, agreed.

Also, I think it’s beneficial to add the relevant metadata – so rather than switch from ShoppingCart#gross_price refers to item more than self (maybe move it to another class?) (FeatureEnvy) to ShoppingCart#gross_price smells of FeatureEnvy I’d keep the reference to item somewhere. Similarly, in Dirty#awful has 4 parameters (LongParameterList) I’d keep the 4, and in Dirty#awful has boolean parameter 'log' (BooleanParameter) I’d keep the log. Thoughts?

Ack as well.

[chastell]

I am working on this, but it seems deciding on just the right format is not simple – for example…

…current:

[1]:Dirty has no descriptive comment (IrresponsibleModule)
[3]:Dirty#awful has 4 parameters (LongParameterList)
[3]:Dirty#awful has boolean parameter 'log' (BooleanParameter)
[3]:Dirty#awful has the parameter name 'x' (UncommunicativeParameterName)
[5]:Dirty#awful has the variable name 'w' (UncommunicativeVariableName)
[3]:Dirty#awful has unused parameter 'log' (UnusedParameters)
[3]:Dirty#awful has unused parameter 'offset' (UnusedParameters)
[3]:Dirty#awful has unused parameter 'y' (UnusedParameters)

…too minimalist:

[1]:IrresponsibleModule: Dirty (Dirty)
[3]:LongParameterList: Dirty#awful (4)
[3]:BooleanParameter: Dirty#awful (log)
[3]:UncommunicativeParameterName: Dirty#awful (x)
[5]:UncommunicativeVariableName: Dirty#awful (w)
[3]:UnusedParameters: Dirty#awful (log)
[3]:UnusedParameters: Dirty#awful (offset)
[3]:UnusedParameters: Dirty#awful (y)

…too verbose:

[1]:IrresponsibleModule: Dirty (has no descriptive comment)
[3]:LongParameterList: Dirty#awful (has 4 parameters)
[3]:BooleanParameter: Dirty#awful (has boolean parameter 'log')
[3]:UncommunicativeParameterName: Dirty#awful (has the parameter name 'x')
[5]:UncommunicativeVariableName: Dirty#awful (has the variable name 'w')
[3]:UnusedParameters: Dirty#awful (has unused parameter 'log')
[3]:UnusedParameters: Dirty#awful (has unused parameter 'offset')
[3]:UnusedParameters: Dirty#awful (has unused parameter 'y')

…a good middle-grond?

[1]:IrresponsibleModule: Dirty
[3]:LongParameterList: Dirty#awful (4 parameters)
[3]:BooleanParameter: Dirty#awful (parameter 'log')
[3]:UncommunicativeParameterName: Dirty#awful (parameter 'x')
[5]:UncommunicativeVariableName: Dirty#awful (variable 'w')
[3]:UnusedParameters: Dirty#awful (parameter 'log')
[3]:UnusedParameters: Dirty#awful (parameter 'offset')
[3]:UnusedParameters: Dirty#awful (parameter 'y')

What do you think?

[troessner]

I like what you labeled with "too verbose" and I don't think it's too verbose!
We should rather be too verbose than concise. Experienced Reek users will mentally strip out the verbose message anyway.
The same way experienced Ruby users will mentally strip:

[1] pry(main)> :foo.omg
NoMethodError: undefined method `omg' for :foo:Symbol
from (pry):1:in `__pry__'

You don't read the message here. You just skim it and then are like "oh, right". But for newbie users this is valuable information.

Let's go with this.

[mvz]

I also like the 'too verbose' version. However, I would leave out the brackets since they're just line noise.

[chastell]

Ok, so what we want to do here is:

• reorder the information,
• simplify it for some smells (like dropping ‘(maybe move it to another class?)’ from UtilityFunction).

I’m a bit lost when it comes to the links, though. Should we add a wiki URL to each line in console output each time, should we keep the current -U / --[no-]wiki-links flag (which defaults to false), or should we keep the flag but default it to true?

[1]:IrresponsibleModule: Dirty has no descriptive comment
[3]:LongParameterList: Dirty#awful has 4 parameters
[3]:BooleanParameter: Dirty#awful has boolean parameter 'log'
[3]:UncommunicativeParameterName: Dirty#awful has the parameter name 'x'
[5]:UncommunicativeVariableName: Dirty#awful has the variable name 'w'
[3]:UnusedParameters: Dirty#awful has unused parameter 'log'
[3]:UnusedParameters: Dirty#awful has unused parameter 'offset'
[3]:UnusedParameters: Dirty#awful has unused parameter 'y'

[1]:IrresponsibleModule: Dirty has no descriptive comment [https://github.com/troessner/reek/blob/master/docs/Irresponsible-Module.md]
[3]:LongParameterList: Dirty#awful has 4 parameters [https://github.com/troessner/reek/blob/master/docs/Long-Parameter-List.md]
[3]:BooleanParameter: Dirty#awful has boolean parameter 'log' [https://github.com/troessner/reek/blob/master/docs/Boolean-Parameter.md]
[3]:UncommunicativeParameterName: Dirty#awful has the parameter name 'x' [https://github.com/troessner/reek/blob/master/docs/Uncommunicative-Parameter-Name.md]
[5]:UncommunicativeVariableName: Dirty#awful has the variable name 'w' [https://github.com/troessner/reek/blob/master/docs/Uncommunicative-Variable-Name.md]
[3]:UnusedParameters: Dirty#awful has unused parameter 'log' [https://github.com/troessner/reek/blob/master/docs/Unused-Parameters.md]
[3]:UnusedParameters: Dirty#awful has unused parameter 'offset' [https://github.com/troessner/reek/blob/master/docs/Unused-Parameters.md]
[3]:UnusedParameters: Dirty#awful has unused parameter 'y' [https://github.com/troessner/reek/blob/master/docs/Unused-Parameters.md]

[troessner]

simplify it for some smells (like dropping ‘(maybe move it to another class?)’ from UtilityFunction).

Why? It's not like we're killing the rain forest by eating more RAM, are we?
I'm on board with "be not super verbose" but I believe we should definitely keep the "(maybe move it to another class?)" message even if it is repeated a gazillion times over the screen since this is relating to Reeks most difficult to fix smells (UtilityFunction and FeatureEnvy).
And the corresponding github issue clearly showed that this is something that even confuses experienced developers.

Should we add a wiki URL to each line in console output each time, should we keep the current -U / --[no-]wiki-links flag

I'd throw it out completely. Experienced Reek user will just skim messages anyway just like you do it with Ruby.

Regarding the URLs: Let's use something like bit.ly and we can clean up the output significantly.

[chastell]

I believe we should definitely keep the "(maybe move it to another class?)" message

Ok, agreed.

I'd throw [-U] out completely.

Hm, I think we can do that no earlier than Reek 4. But maybe we can reverse the default in Reek 3?

Let's use something like bit.ly and we can clean up the output significantly.

I’d really rather not – URL shorteners are both DNS waterboarding, they tend to rot and we don’t have control over them. But if @mvz is for this then I won’t veto. :)

One last question – sorry for being so ping-pongish about it, but this change means going through all of our very detailed cucumber scenarios and changing them, so I’d rather do it once ;) – should we sort the output by smell names now? IMHO it would be both much more pleasant to the eye and more useful (as in, ‘I’ll go and fix all of the UnusedParameters today’).

[mvz]

I'd like to keep -U semantics as it is for now and revisit that part later. Since we actually have the documentation right here in the repository, we could do fun things like reek --explain DataClump and have it actually print the docs. But as i said, let's revisit that in a later issue.

I'm for cleaning up the URLs, but would not like to use a shortener. Perhaps we should get an actual website? But again, that's probably for a separate issue.

[troessner]

I’d really rather not – URL shorteners are both DNS waterboarding, they tend to rot and we don’t have control over them. But if @mvz is for this then I won’t veto. :)

Good point, let's not do that.

Hm, I think we can do that no earlier than Reek 4. But maybe we can reverse the default in Reek 3?

Kind of depends: Do we consider a change in a non-vital cli switch a breaking change? I'd just go ahead and do it.

I'd like to keep -U semantics as it is for now and revisit that part later.

Ok, agreed.

– should we sort the output by smell names now?

But sort within all the smells for one given source, not across all sources, right? If yes then I think that's an awesome idea.

[mvz]

should we sort the output by smell names now?

Probably best to sort the detectors by name in that case. The output will be sorted automatically.

[chastell]

Ok, one more (ha!): should we sort directly by smell_type or group by smell_category?

Going with category first is logically more correct, but I tried it and it’s confusing: ‘why are the smells mostly sorted by name, but not always?’…

[troessner]

Let's stick with smell_type :)

[chastell]

Whew, that was a lot of Cucumber features editing. Anyways: #765, and I’m very open to any change proposals (but they may take some time). :)