Suppress or consolidate smells for functions pending implementation

[l4cr0ss]

I did a quick search of the issues, both open and closed, but didn't see something like what I'm proposing.

The problem I am having is that reek is throwing up warnings for scaffolding code that hasn't yet been fleshed out. I've included an example below.

I'd like a mechanism that will let me suppress all warnings for a function in the particular instance that the function is not yet implemented.

It merits consideration because the alternative is going in by hand and notating every method (of which there can be many) with each of the specific errors reek is detecting for that instance. It's an ugly workaround that goes against the spirit of the tool.

I propose that we name it PendingImplementation. A wildcard operator would be just as effective, but what I like about PendingImplementation is that it can't be abused to silence the output of reek without drawing attention to itself.

Example of the problem:

# TODO: Implement this method
def my_undone_function(param1, param2, param3)
    raise NotImplementedError.new("this function must be implemented") 
    super("currently unreachable, implementation relevant code") 
end

Generates:

Errors on modified lines:
  ...: UnusedParameters: MyFictionalParent::MyFictionalChild#my_undone_function has unused parameter 'param1' [https://github.com/troessner/reek/blob/v5.0.2/docs/Unused-Parameters.md]
  ...: UnusedParameters: MyFictionalParent::MyFictionalChild#my_undone_function has unused parameter 'param2' [https://github.com/troessner/reek/blob/v5.0.2/docs/Unused-Parameters.md]
  ...: UnusedParameters: MyFictionalParent::MyFictionalChild#my_undone_function has unused parameter 'param3' [https://github.com/troessner/reek/blob/v5.0.2/docs/Unused-Parameters.md]

Potential solution:

...
# :reek:PendingImplementation
def my_undone_function(param1, param2, param3)
    ...

Potentially generates:

Errors on modified lines:
  ...: PendingImplementation: MyFictionalParent::MyFictionalChild#my_undone_function has been marked as pending implementation [https://github.com/troessner/reek/..<relevant_doc>.md]

Thanks for your time. Let me know if a pull implementing this is something that will be considered.

[mvz]

The problem I am having is that reek is throwing up warnings for scaffolding code that hasn't yet been fleshed out.

When exactly is this a problem? It is natural for code that is a work-in-progress to have some code smells. How is this hampering your workflow?

Errors on modified lines:

What tool are you using that generates this text?

[l4cr0ss]

Please permit me to answer your questions in reverse order.

Errors on modified lines:

What tool are you using that generates this text?

That will be this tool, https://github.com/sds/overcommit. It offers a mechanism by which to manage git hooks. Here, the git pre-commit hook is kicking off the code quality suite my organization employs. The output is being categorized multiple ways, the one being shown indicating that this was a change introduced by the commit being performed.

When overcommit detects that prehook checks have failed, it prevents git from completing the commit. That means for developers to commit, they must make the checks pass.

The problem I am having is that reek is throwing up warnings for scaffolding code that hasn't yet been fleshed out.

How is this hampering your workflow?

A big chunk of my time during the day is spent reviewing code. We get lots of pull requests and those pulls come from teams of developers of wide-ranging skill levels. It has been a big win for the organization to include reek (among other things) in our pre-commit hook script, helping to improve the signal-to-noise ratio for reviewers and developers alike.

The fact of the matter is that some developers are better at writing "clean" code (from the perspective of the code quality suite) than others. Sometimes a developer feels they are "under the gun", and rather than making their tests pass they signal OVERCOMMIT_DISABLE=1 and push the code straight into the PR queue. Likewise, sometimes a reviewer feels the same pressure, and pushes something through code review that needed more attention.

Alert fatigue is a very real thing, and it benefits both the reviewer and the developer to be able to decrease the amount of noise presented to them when its time for them to commit. It increases the chance something big gets caught during code review, and it makes for a more encouraging - or at least less confusing - developer experience.

tldr; It is much easier for me to teach a developer to respect the code quality machinery when the warnings it produces are semantically correct.

The problem I am having is that reek is throwing up warnings for scaffolding code that hasn't yet been fleshed out.

When exactly is this a problem? It is natural for code that is a work-in-progress to have some code smells.

I agree - code in progress naturally tends to have some smells and the current smell suppression system is working very well at addressing those instances.

But it becomes a problem, for example, in a project with the aim of refactoring highly-coupled spaghetti code so that it leverages inheritance in a sensible way. The reality is that work can't stop just because we need to refactor, so, we write the design, we implement the design, and we start moving functionality out of the legacy code and into the new code piece by piece.

Each sprint makes a little more progress, but each feature to be refactored has to go through the entire SDLC, and invariably little kinks show up here and there. These stub functions have a longer life than one might think, and it creates a lot of opportunity for confusion. A new developer, (or architect!) without the domain knowledge and contextual information possessed by a developer having more experience with the project, might take the reek suppression directives to indicate an honest problem and take it upon themselves to "fix" the problem, wasting time and potentially developing a resentment toward the team, the tool, or both. I believe reek stands to benefit from a feature by which a function can be marked as pending.

tldr; Squelching N+1 code smells for a function is itself a code smell, and isn't the same as marking a function "not ready" for smell detection.

Thanks again for your time here.

[l4cr0ss]

Just want to follow up and ask again: will a pull implementing this feature be considered?

[mvz]

Hi @l4cr0ss, I'll try to get back to you on this this weekend.

[mvz]

When overcommit detects that prehook checks have failed, it prevents git from completing the commit.

I do not recommend having commits fail due to Reek issues. Code smells may take several refactorings to resolve, and a very important aspect of refactoring is being able to take small steps, and commit intermediate results, as well as the state of the code that was working but smelly. It is much safer to check for smells at the pull request level. This can be automated using CI or automatic reviewing tools.

A new developer, (or architect!) without the domain knowledge and contextual information possessed by a developer having more experience with the project, might take the reek suppression directives to indicate an honest problem and take it upon themselves to "fix" the problem, wasting time and potentially developing a resentment toward the team, the tool, or both.

You will have to communicate the state somehow. When you introduce a generic suppression, that will also have to be removed again at some point. When that time would be, would have to be communicated somehow. In either case (suppressing several smells, or all smells at once), that communication will have to happen outside of Reek. A simple comment added to the method describing its state should be effective.

That said, a suppressing comment that still causes Reek to output one smell (PendingImplementation) would cause your developers to apply OVERCOMMIT_DISABLE more often. This may not be the pressure you want to apply.

I'm more positive toward the possibility of suppressing all Reek output for a given method or class (your 'wildcard' option):

# NOTE: This method is under construction, don't try to refactor yet.
# :reek:*
def my_smelly_method(foo, bar)
  # smelly code here
end