[erlang-questions] Feedback on inaka/erlang_standards
Brujo Benavides @ Inaka
elbrujohalcon@REDACTED
Tue Sep 9 15:19:16 CEST 2014
Hi all,
Long mail ahead, fasten your seatbelt ;)
Before addressing the individual points in Loïc’s mail, I want to get a couple of facts about elvis[1] and erlang_standards[2] straight, to eliminate (or alleviate) some doubts about them:
First of all, a note on their purposes:
1. Elvis is an open-source configurable and extensible style-checker, intended to be used by any Erlang programmer in the world, therefore its “standards” (a.k.a. rules) are as flexible as they may be (e.g. the rule is defined as “nesting_level, [MaxNestingLevel]” so that each dev can pick their favourite MaxNestingLevel)
2. erlang_standards is the set of standards that we (at Inaka) enforce in our Erlang code. Since they reflect _our_ take on style rules, they’re not as flexible as elvis’ rules (e.g. the rule says "Try not to nest more than 3 levels deep.”, i.e. MaxNestingLevel = 3 for us).
Now, one of our goals when building elvis was for it to be as flexible as possible. To achieve that goal, we provided a couple of ways in which devs can configure it, namely:
- if you use elvis as a command line tool, you MUST provide a configuration file (there is an example on the elvis repo [3], but there are NO default values). That way you MUST decide which rules you want to enforce explicitly.
- those rules in elvis.config, are just MFAs that all happen to be in the elvis_style module, but can actually be in any module you want (granted, if you want to apply your own rules you need to run elvis in an Erlang shell with access to your modules, but I already opened an issue there to see if we can find a way to handle the same behaviour in the escript version)
- if you use elvis as a server integration with github [4] there is a default set of rules, BUT you can override it by simply placing an elvis.config file in the root folder of your repo. Granted: you can’t run rules defined in your own modules there, sorry about that :) You can submit them as pull requests to elvis and, once they’re merged, they’ll soon be available at the service ;)
So, in a nutshell: Elvis should fit your needs and your rules, not the ones at Inaka. If you think that’s not the case, and it needs further integration/configuration/simplicity… please open an issue on github or better send us a PR.
Now, moving on to erlang_standards. As much as I would love that repo to become a centralised list of standards for the whole erlang community, I would argue that if that’s going to be the case, it shouldn’t be in Inaka’s github, but rather on erlang’s github (https://github.com/erlang/erlang_standards) or something similar (BTW: I’m totally fine giving you ownership of the repo, OTP guys… I would then fork it at Inaka and adapt the rules that we, at the company, want to enforce but are not massively accepted by the community). In the meantime, as I said, it will only reflect the ideas of the ~8 erlangers that were/are working at Inaka. We accept contributions because we love good and constructive debate, but we don’t expect anybody outside our company (well… we don’t even expect everybody INSIDE our company) to like them :)
With all that said, and taking off my Inaka’s CTO hat, I’ll address Loïc’s points with my very personal opinions that in no way are supposed to represent Inaka’s vision (should that even exist):
> Spaces over tabs, 2 space indentation.
I agree: whatever floats your boat. Elvis has just the no_tabs rule because writing a proper "indentation, [spaces, 2]” rule proved way harder than we originally thought. If somebody out there is talented or resourceful enough to write it, we will LOVE to have it added to elvis.
> Surround operators and commas with spaces.
Again, I totally agree. But, again, having such a rule as you describe would be awesome… we just wrote a simpler one that is *just good enough*, but can use lots of improvement, for sure.
> Try to always separate PRIVATE and PUBLIC functions in groups, with the public ones first, unless it helps readability and code discovery.
I agree on the OOP'ism (facepalm). I’ll open an issue to change that :P
I also agree that many modules are better structured as you describe, that’s why the "unless it helps readability and code discovery” is there.
This rule, as all other very subjective and in-the-eye-of-the-code-reviewer rules, will never reach elvis for that reason exactly.
> Don't design your system using god modules (modules that have a huge number of functions and/or deal with very unrelated things)
I agree that some modules are the exception to this rule and I reckon that just checking the number of exported functions on them is not exactly a good measure of the fact that they’re god modules or not. For instance, you can have a god module that’s just a gen server with a few exported functions but dozens of handle_call and handle_cast clauses: it will be undetected by elvis, but it should be detected by the code reviewer. So, yeah… the god_modules rule is not actually detecting the existence of god modules, because probably… no rule can. It just detects one kind of big modules and warns you about them so that you can check yourself if it’s in fact a god module or not. In cowboy’s case, you should just remove it from your elvis.config ;)
> Don't write the same code in many places, use functions and variables for that
I agree completely with what you said. That’s why this is another one of those VERY subjective rules and, as the README states it’s here (instead of being treated as "just a good idea” to let reviewers reject PRs that include the same code several times or PRs that re-implement something that they know it's already done somewhere else
> When having lots of modules, use subdirectories for them, named with a nice descriptive name for what that "package" does.
I also agree, sometimes those things are better off as applications, but we have apps with cowboy handlers and sumo models that are better placed in “handlers" and “models” folders. But yeah… I would add the part about considering moving stuff to different applications to the rule. [5]
> Don't write spaghetti code (A list comprehension with a case inside, or blocks with begin/end, and nested stuff)
I used to be a big fan of list comprehensions with things inside them as the example [6], which incidentally is based on a code written by me, will show soon, until I had to explain parts of my old code to new devs here. They stared at me for hours while I stared at my old code for the same time :P
Marcelo Gornstein (hopefully he’s reading this as well) was one of the victims and, in the process of understanding what I had written, he taught me how to write it more clearly and manageably. That’s where this rule came from. And again… it may never be checked by elvis, because it’s kinda subjective.
> Don't use if .
I’m a very anti-if kinda guy, and it comes from my years working on OOP languages like SmallTalk. But I also know how this subject may spark lots of flame-wars, so I refrain from discussing it. The three or four last times when I enrolled in a debate about the subject it all boiled down to me dissecting other people’s ifs into functions or cases or other constructs that (at least to my eyes) looked better and more manageable, but I don’t expect anybody to abide to that rule except for Inaka employees in Inaka projects, of course ;)
> Macros should be named in ALL_UPPER_CASE
I’m with you 100%… I HATE macros, except for ?MODULE, ?MODULE_STRING, ?LINE, ?FILE, etc. But I couldn’t get that far for Inaka’s standards so the compromise was to, at least, write them in all upper case :P
> Write the -spec's for your public fun's, and for private fun's when it adds real value for documentation purposes. Define as many types as needed.
I agree with you on this one as well, but Inaka’s consensus was to require devs to write specs for functions with too generic names like get_events(Account, Day, Order). ¯\_(ツ)_/¯
> Module comments go with %%%, function comments with %%, and code comments with %.
Yeah, as Ivan said: this rule is here to cope with Emacs users :P (Yeah… I’m not one of them, I’m a happy sublimer :P)
Keep in mind that this rule it’s not in the list of actual (i.e. strict) rules, but in the list of “Suggestions & Great Ideas” which is the list of "nice things to keep in mind but nobody will complain if you don’t”.
> Try to write functions with a small number of lines. 12 lines per function except for integration tests is a good measure.
If you’re writing an init function or main loop and it has more than 12 lines (that should be “expressions", BTW [7]), then you probably can find a way to divide that function into a couple of different ones, each one doing a smaller (and more manageable) part of the work. In the same sense as the if’s rule… I wouldn't expect everybody to agree on this one. I know it can spark flame-wars as well, and (in my case, and I was not the only one advocating for this) it also comes from my background as a SmallTalk programmer.
This rule is also out of the stricter part, like the one above.
> Help dialyzer and xref as much as you can, so that they can work for you
Yeah, this one is as vague as it can be. It may be removed from the list moving forward because it’s impossible (even for human reviewers, and not just elvis) to check. That’s why, also, it’s not in the list of actual rules, but in the list of “Suggestions & Great Ideas”.
> When having many nested "include files", use -ifndef(HEADER_FILE_HRL) .... -endif so they can be included in any order without conflicts.
…aaaaaand I agree with you on this one, too. And I think your suggestion is really valuable, please add it as an issue for elvis and, if you like, as a rule suggestion [8] for erlang_standards.
So, to sum up: I basically agree with you on almost everything but two very-subjective very-flame-war-inducing rules which I don’t really want to discuss (one of them it’s not even an actual strict rule). I also want everybody to notice how elvis is configurable and how erlang_standards is Inaka’s list and I would LOVE to have a similar one at [2].
All that said, thank you for reading up to this point, I know it was very long email and therefore I leave you with the best animated gif in the history of animated gifs: http://www.reactiongifs.com/r/best-gif-ever1.gif
Enjoy!
[1] https://github.com/inaka/elvis
[2] https://github.com/inaka/erlang_standards
[3] https://github.com/inaka/elvis/blob/master/config/elvis.config
[4] https://elvis.inakalabs.com
[5] https://github.com/inaka/erlang_standards/issues/16
[6] https://github.com/inaka/erlang_standards/blob/elbrujohalcon.13.provide_examples_in__erl_files/src/spaghetti.erl
[7] https://github.com/inaka/erlang_standards/issues/17
[8] https://github.com/inaka/erlang_standards/blob/master/CONTRIBUTING.md
> On Sep 9, 2014, at 07:53, Ivan Uemlianin <ivan@REDACTED> wrote:
>
> I like the idea of a configurable coding standard checker, if that's what elvis is, and as long as team members read and discuss (and agree on) the team's "elvis.config". Discussing and agreeing on standards is as important as following them. Each team will have their own conventions: their own boats to float.
>
> As for particular points:
>
> - I tend to avoid if, and macros as far as possible: two fewer bits of syntax to remember. I notice I'm starting to avoid case too.
>
> > Module comments go with %%%, function comments with %%, and code comments with %.
>
> This is perhaps following emacs erlang-mode behaviour (which auto-intends comments differently depending on how many %s start the line).
>
> Discussing coding style is worthwhile. I think a monolithic One True Style is less useful, especially in a programming culture as articulate and reasoned (and occasionally stiff-necked) as erlang's.
>
> Best wishes
>
> Ivan
>
>
> On 08/09/2014 22:47, Loïc Hoguin wrote:
>> Hello,
>>
>> I want to initiate a larger discussion that Twitter allows for the inaka/erlang_standards repository. I hope you don't mind that I do it here, Github issues is quite terrible for discussions and I feel that talking about these things with a larger number of people can bring better feedback.
>>
>> I will take all points listed in the README at https://github.com/inaka/erlang_standards and comment if I have something (hopefully) interesting to say about it.
>>
>> I know they are "Inaka's coding standards" but they are also used for the Elvis coding standard checker project which would be interesting to try and integrate as part of more people's workflow (regardless of projects using the same standards or not, simply checking that contributions respect it is already quite nice to have).
>>
>> An example output can be seen there: https://gist.github.com/elbrujohalcon/332db8bc343f81235462
>>
>> So let me start now.
>>
>> > Spaces over tabs, 2 space indentation.
>>
>> Whatever floats your boat. I just want to point out here that 2-4 space vs tab vs mixed never lead anywhere. My problem with it though is that the Elvis tool has an option "no_tabs". That's very opinionated, instead you should have an option that allows you to define indentation. For example I want to know if people use spaces instead of tabs in my projects.
>>
>> > Surround operators and commas with spaces.
>>
>> As much as I like having these with spaces around, I have an issue with that. I feel that the default should be "whatever Erlang is doing". For example maps, it does "#{a => b,c => d}". The default your tool should do is exactly what Erlang does, because you can then just copy paste everything and not have to worry. We could extend that logic to doing what erl_pp does but people really like their tabs/spaces/goats so that'd probably be taking it a bit too far.
>>
>> > Try to always separate PRIVATE and PUBLIC functions in groups, with the public ones first, unless it helps readability and code discovery.
>>
>> I have to disagree with that. This is what I have done for a *long time* but I experimented and changed my mind recently.
>>
>> What I do now is for *each* exported function (nice OOP language btw), start with the exported function, then the internal functions it calls, and then the unit tests (and performance tests). Then do the same for the next exported function and so on. It doesn't fit all kinds of modules (of course!) but for modules that aren't doing processes stuff (like gen_servers) it's very nice.
>>
>> For example: https://github.com/ninenines/cowlib/blob/master/src/cow_qs.erl - if that module was doing the old public, private, tests, it would be pretty much unreadable. Having the unit tests right there with the code is very nice because you can easily see what the code takes and returns.
>>
>> > Don't design your system using god modules (modules that have a huge number of functions and/or deal with very unrelated things)
>>
>> Well that's fine if your problem domain is simple, but not so much otherwise. The problem with your approach comes from being able to set a maximum number of functions project-wide. In Cowboy for example most modules are small, and cowboy_req is the exception. I know cowboy_req has a lot of exported functions, I don't want a tool to tell me that. It's meant to be this way.
>>
>> > Don't write the same code in many places, use functions and variables for that
>>
>> I have for a long time been a proponent of applying DRY only when this code exists more than 2 times, and the code is significant enough. I care little about repeated code that's 2 lines long for example. I don't have hard limits there but it would be nice to analyze a few projects with different settings and find what's the most useful.
>>
>> > When having lots of modules, use subdirectories for them, named with a nice descriptive name for what that "package" does.
>>
>> Sounds like you need to separate into different applications.
>>
>> > Don't write spaghetti code (A list comprehension with a case inside, or blocks with begin/end, and nested stuff)
>>
>> Not what I call spaghetti code? I'm not even sure what this is about. List comprehensions with a case or begin/end inside can be immensely useful. Especially the begin/end ones.
>>
>> > Don't use if .
>>
>> I would say the opposite. ROK has already argued on that, no need to duplicate. :-)
>>
>> > Macros should be named in ALL_UPPER_CASE:
>>
>> I would say "Don't use macros" instead. I would love the tool to tell me when it sees macros that aren't ?MODULE (and a way to whitelist a few small ones).
>>
>> > Write the -spec's for your public fun's, and for private fun's when it adds real value for documentation purposes. Define as many types as needed.
>>
>> Honestly I'd remove the comment about internal functions (wow OOP man), they're generally not very useful, unless Dialyzer complains.
>>
>> > Module comments go with %%%, function comments with %%, and code comments with %.
>>
>> Or you could stop worrying and enjoy life. Seriously that's some nitpicking you got there. :-)
>>
>> Instead of saying that you should probably worry that people *actually* comment their code.
>>
>> > Try to write functions with a small number of lines. 12 lines per function except for integration tests is a good measure.
>>
>> 12, seriously? If I'm writing a game the init function or main loop function will hardly be less than 20 lines.
>>
>> > Help dialyzer and xref as much as you can, so that they can work for you
>>
>> Like with most things in life, there's an optimal middle ground. Don't go out of your way to make tools happy. Tools are supposed to help you not hinder you.
>>
>> > When having many nested "include files", use -ifndef(HEADER_FILE_HRL) .... -endif so they can be included in any order without conflicts.
>>
>> I'll need an option that complains when -include is used (again, with whitelist, probably never warn about file.hrl either). :-)
>>
>> That's it. Hope to spark some (not too pointless) discussion.
>>
>> Enjoy!
>>
>
> --
> ============================================================
> Ivan A. Uemlianin PhD
> Llaisdy
> Speech Technology Research and Development
>
> ivan@REDACTED
> www.llaisdy.com
> llaisdy.wordpress.com
> github.com/llaisdy
> www.linkedin.com/in/ivanuemlianin
>
> festina lente
> ============================================================
>
> _______________________________________________
> erlang-questions mailing list
> erlang-questions@REDACTED
> http://erlang.org/mailman/listinfo/erlang-questions
More information about the erlang-questions
mailing list