Create CondFinalCondition readability check

[Stratus3D]

This PR adds a check named Credo.Check.Readability.CondCatchallTrue that checks for conds that end in a catchall clause that use a literal value other than true. I created this check to catch violations of the rule about catchall cond clauses specified in the Christopher Adam's style guide (https://github.com/christopheradams/elixir_style_guide#true-as-last-condition). See the tests for examples.

Context:

• How do we use this style guide in an automated way? christopheradams/elixir_style_guide#143 (comment)

[Stratus3D]

Open to suggestions on what to name this check. I had a hard time picking a name and I'm not sure this name is best.

[Stratus3D]

Returns true of the clause condition is a catchall that isn't the literal value true.

[rrrene]

Great 👍

Since the Elixir website & documentation call this the "final condition", we should name this CondFinalCondition.

Also, I do think the true should be a config parameter for this check. I can't think of a reason why we would exclude people using :else from the ability to check for it (although true should be the default to match the linked styleguide). 👍

[Stratus3D]

Great 👍

Since the Elixir website & documentation call this the "final condition", we should name this CondFinalCondition.

Also, I do think the true should be a config parameter for this check. I can't think of a reason why we would exclude people using :else from the ability to check for it (although true should be the default to match the linked styleguide). 👍

PR updated!

[Stratus3D]

@rrrene this is ready for review again!

[Stratus3D]

@rrrene any chance you can take a look at this PR again?

[rrrene]

Apologies, I'm having the first cold in 2.5 years and am no longer used to that *%&/!.

Did you use a formatter on the code?

[rrrene]

If the clauses "above" this one are "catch alls" for all cases, what are examples where we need this clause?

[Stratus3D]

This is for a non catch-all clause. A contrived example:

 cond do
   n > 5 ->
     "greater than 5"
   n >= 0 ->
     "positive"
   n < 0 ->
     "negative"
end

The last clause is not a catchall and is not composed of a single literal. I will add a comment for this final clause. Let me know if you think I should rename this function.

[rrrene]

Please rename this function to something that gives a hint at what it does.

[Stratus3D]

PR updated. Let me know if the new name is acceptable.

[rrrene]

Why does this always return true, no matter what I specified in the config?

[Stratus3D]

This function returns false when the value variable bound by the match of the first argument ({:->, _meta, [[value], _args]}) matches the second argument value. The second argument passed to this function is value specified in the config.

[rrrene]

Why is the trigger for the issue always "cond"? The trigger is the word in the code that "triggers" the issue and that should be the "wrong" final condition, e.g. using :else when using true is expected.

[Stratus3D]

PR updated. The trigger is now the literal catchall value, or in the case of maps, tuples, and integers, the atom :map, :tuple or :integer. Let me know if this is acceptable.

[rrrene]

As I mentioned before

The trigger is the word in the code that "triggers" the issue

which means the actual string that should get squiggly lines in your editor.

[Stratus3D]

I'm sorry about that. I don't know how to do that but I'll see if I can figure it out. Are there any other checks that I could refer to that do this?

[Stratus3D]

PR updated. I have added assertions to the tests for this check to verify that the clause expression that triggers issue is what is set for the issue's trigger parameter.

[Stratus3D]

Apologies, I'm having the first cold in 2.5 years and am no longer used to that *%&/!.

Did you use a formatter on the code?

Sorry to hear you have been sick. Thanks for the review and thank you for all your work on credo!

I forgot to run the formatter when I opened the PR. I ran it and pushed the reformatted code. Everything is ready to review again.

[Stratus3D]

I have updated the PR again based on your reply to one of my comments about the issue trigger value. The trigger is now set to the string of code that triggered the issue. The PR is ready to review again.

[Stratus3D]

Any chance this can get merged soon?

[rrrene]

I have added assertions to the tests for this check to verify that the clause expression that triggers issue is what is set for the issue's trigger parameter.

This is good in principle, but it does not work if I modify your tests to something that might very well happen in real life (a dangling space somewhere):

*..................................................................................................................................................................................................................................................................................................................................

  1) test it should report conds with a last condition that is a list literal (Credo.Check.Readability.CondFinalConditionTest)
     test/credo/check/readability/cond_final_condition_test.exs:141
     Assertion with == failed
     code:  assert "[:else ]" == trigger
     left:  "[:else ]"
     right: "[:else]"
     stacktrace:
       (credo 1.6.2) lib/credo/test/assertions.ex:30: Credo.Test.Assertions.assert_issue/2
       test/credo/check/readability/cond_final_condition_test.exs:160: (test)

.

  2) test it should report conds with a last condition that is a tuple literal (Credo.Check.Readability.CondFinalConditionTest)
     test/credo/check/readability/cond_final_condition_test.exs:165
     Assertion with == failed
     code:  assert "{ :else }" == trigger
     left:  "{ :else }"
     right: "{:else}"
     stacktrace:
       (credo 1.6.2) lib/credo/test/assertions.ex:30: Credo.Test.Assertions.assert_issue/2
       test/credo/check/readability/cond_final_condition_test.exs:184: (test)

.......................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................

[Stratus3D]

@rrrene I copied what was done in the existing checks. I used Macro.to_string(ast) to convert the AST back into the original expression and passed that into the issue as the trigger value.

Can you provide a suggestion on how to improve this? Happy to change things but not sure how.

[Stratus3D]

@rrrene, I'd like to get this merged, can you show me how to address this in credo? I'm not sure I can get the cond clause with whitespace in credo.

This is good in principle, but it does not work if I modify your tests to something that might very well happen in real life (a dangling space somewhere):

Can you provide a suggestion on how to improve this? Happy to change things but not sure how.

Thanks!

[Stratus3D]

Re-opening this PR as I closed it in error. @rrrene happy to make any changes to get this merged!

[Stratus3D]

@rrrene you've merged several of my checks but not this one. If you can provide any info as to why this hasn't been accepted yet I will be happy to make any necessary changes (or close it permanently if that is needed).

[rrrene]

If you can provide any info as to why this hasn't been accepted yet

Hi, sorry for being a slow responder. This check won't get merged as long as it assumes that people write code the same way that Macro.to_string/1 translates ASTs. This is just an unrealistic expectation.

I do not have an "obvious" solution for your specific problem here (a.k.a. I'm not trolling you).

[rrrene]

I am closing this for age/inactivity.

If you find a way to make the final changes, please feel free to re-open this issue at your discretion.

[Stratus3D]

Hi @rrrene , sorry for the late reply here.

Hi, sorry for being a slow responder. This check won't get merged as long as it assumes that people write code the same way that Macro.to_string/1 translates ASTs. This is just an unrealistic expectation.

Are you referring to this line? https://github.com/rrrene/credo/pull/935/files#diff-cbac4e2c2b102d2eba670658d03634fb4b21022007e98f39f5b7bccd674e52daR57 The Credo.Check.Warning.ExpensiveEmptyEnumCheck already uses Macro.to_string/1 to translate AST into code for the error (see https://github.com/rrrene/credo/blob/master/lib/credo/check/warning/expensive_empty_enum_check.ex#L66).

Is there a reason this is problematic? I understand the code may not be formatted exactly as the user typed it in, but I'm not sure this will be a big problem as we do output the line number as well, so the user should be able to locate the problem without issue. Remember, this AST in this case is only ever going to be a catchall clause literal value. So for example, :else, %{random: value}, [], [1,2,3], etc... it'll never be an anything other than a literal. While it's true the user may have formatted the literal differently, it I don't think it'll be a big problem in practice. The only alternative to I see to this would be to remove the trigger from reported issues entirely, which probably is not desired.