New `elvis_style` rule: `max_function_clause_length`

[bormilan]

Description

I added a new rule called max_function_clause_length. The old rule max_function_length stays the same, it checks the length of an entire function, but now we can check the size of the clauses separately.

Closes #335

• [x] I have read and understood the contributing guidelines

[bormilan]

I'm thinking about merging the two rule codes because they are so similar. I should make only one function, that gets parameters(e.g. function or clause), and then call that with the appropriate values.

But on the other hand, I think it's more readable and simple.

What do you think about it?

Oh, and I don't know that the error message is well enough.

[paulo-ferraz-oliveira]

Thanks much. In general, this looks solid. I propose a few tweaks. From my end, I prefer it if you don't resolve the comments while you handle them, since it allows me to check your reply easier (and not having to press "Show resolved" all the time).

[bormilan]

I'm thinking about that, we should check the error messages during the tests, to make sure we caught the error message we want.

We should make a function that takes the expected message/messages and returns a boolean based on the result.

If you think it's useful I would be happy to implement it in another issue maybe, or if not I will just write this test by checking the messages by hand.

[elbrujohalcon]

In general, I don't like to check error messages (or other strings, FWIW). They're too variable. But in this case, with the Nth/nd/rd thingy… it may be worth adding a test specifically for that. Something like having limit => 1 and a function with 25 clauses, all of which have 2 lines in length.

[bormilan]

I have a short time now, that's why it's half done.

I want to add better test cases, and unit tests for the case_num function.

And the "function name" is now bad, so maybe I need to parse the clause_nodes and add the function name and arity to it.

[bormilan]

Now, I fixed it a little bit, I hope you find it better too.

[elbrujohalcon]

Sorry, @bormilan … but I still fail to see why you need all the recursiveness in that function. In the end it just needs to return a number in string format, doesn't it?

[bormilan]

Do you mean why do I need the function clause_num alongside the fun parse_clause_num? Because I need to figure out the clause's number that is not stored in the data of the node, so I need to calculate it by hand. We have a list of all the clauses in the module, so I need to go through them with a lists:foldl and calculate the numbers at every node.

[elbrujohalcon]

Do you mean why do I need the function clause_num alongside the fun parse_clause_num? Because I need to figure out the clause's number that is not stored in the data of the node, so I need to calculate it by hand. We have a list of all the clauses in the module, so I need to go through them with a lists:foldl and calculate the numbers at every node.

Ooooh… but why isn't it enough to do Acc + 1 on each iteration of the fold. I mean… Clauses0 has all the clauses of the function, right? You process them one after the other… you can only raise a single error per clause… so each clause either produces an error or not and to know which clause is each clause, you start with 1 and then you do +1 on each iteration and that's it. Am I missing something?

[bormilan]

Oh maybe my fault was that I searched all the clauses in the root, but I might search for them only in the function node. So I need to search for the functions, and in an inside recursive "iteration" I run through the clauses. This way I will end up with a two-deep algorithm, but I don't need to calculate the clause numbers like I did.

[bormilan]

This solution would be good in my opinion, but because the fact that I need the function name and arity too, I need to switch to the two-deep solution. When I started this looked better because of simplicity, but I was wrong now I see. (sad)

[bormilan]

Now I made the new version, with this I don't need that plus function for the clause numbers. I made a new test, and before merging, I want to make an assertion about the messages(to prove that clause numbers and the message itself are correct) if you think it's good too.

(and of course, I will delete the comments)

[elbrujohalcon]

I'm approving. I think this is mergeable as it is, but if you feel like adding a test for the clause numbers, running elvis on a file like this should produce one error per clause.

-module( … ).

-elvis([{max_length_function_clause, #{max_length => 0}}]).

-export([clause/1]).

clause(1) -> "1st";
clause(2) -> "2nd";
…
clause(111) -> "111th";
clause(112) -> "112th";
clause(113) -> "113th";
–
clause(125) -> "125th".

[bormilan]

Thank you so much!

[paulo-ferraz-oliveira]

I tweaked the PR title since this'll go directly into the generated release Changelog.

[paulo-ferraz-oliveira]

I left a couple of comments above, that should be handled; after that we're good to go. Thanks.

[bormilan]

I'm Ok, I guess, but I don't know what changed since my last review, since all the commits were squashed.

Oh, sorry next time I will wait with it till the very end.

[elbrujohalcon]

Almost there, @bormilan

[elbrujohalcon]

Thanks, @bormilan !!! Amazing work.