inaka / elvis_core

The core of an Erlang linter
Other
61 stars 56 forks source link

Do not apply atom_naming_convention over function names, only atoms used as atoms #204

Open nixxquality opened 3 years ago

nixxquality commented 3 years ago

https://erlang.org/doc/man/zlib.html

The functions here are called inflateInit, inflateEnd, et.c. which angers Elvis.

Any thoughts on this? I see this as the current workaround

decompress_data(Data) ->
    Bin = base64:decode(Data),
    Z = zlib:open(),
-   ok = zlib:inflateInit(Z),
+   ok = zlib:'inflateInit'(Z),
    Deflated = zlib:inflate(Z, Bin),
-   ok = zlib:inflateEnd(Z),
+   ok = zlib:'inflateEnd'(Z),
    ok = zlib:close(Z),
    jsone:decode(iolist_to_binary(Deflated)).

But I don't like working around linters.

Should atoms from the standard library be exempt?

elbrujohalcon commented 3 years ago

You don't need to work around like that. Just add an -elvis attribute to your module with the proper regex to handle the conflicting atoms.

nixxquality commented 3 years ago

Sure, but I would still be working around the linter in my code where the fault lies somewhere else.

elbrujohalcon commented 3 years ago

Yeah. But we can't force our style on other people's code 🤷‍♂️. Today is otp's zlib. Tomorrow it will be some other library. The best we can do is respect the standard ourselves and ignore the code in our deps. Right?

elbrujohalcon commented 3 years ago

Maybe what elvis can do is to ignore atoms that are used as function names for this rule.

That would not be a bad idea.

nixxquality commented 3 years ago

Yes, my point wasn't that this style should be forced on other people's code, rather that it would be nice if it was smart enough to recognize cases where it's out of your hand.

elbrujohalcon commented 3 years ago

Got it! Yeah... Skipping function names would achieve that goal, at least in this context.

paulo-ferraz-oliveira commented 3 years ago

Should atoms from the standard library be exempt?

If there's an easy way to list them automatically, sure...

Maybe what elvis can do is to ignore atoms that are used as function names for this rule.

Not sure what the implications of this statement are, but function names come in many forms, in the code:

Though there's no (real) convention I know of, around function names (http://www.erlang.se/doc/programming_rules.shtml#REF17122), the choice that elvis presents is consistent with the majority of naming we find in the wild. I've also had to add a rule exception to some of our lib.s. The same happens for eldap and quite a few functions.

@elbrujohalcon, let me know if you want to discuss this further; I wouldn't mind implementing the exception if that's the conclusion we reach. Or even accept a pull request given pointers to @nixxquality on where this should be changed.

paulo-ferraz-oliveira commented 3 years ago

Hm... wait, I think I see what you mean here, atom_naming_convention is being applied over a function name (which should be Ok). Makes sense, in many cases, for example if the function name is declared first as an atom and then used later, i.e.

Mod = zlib,
Fun = inflateInit,
Mod:Fun(Z)

In that case, if changes apply (to elvis) we should probably also reconsider working on (maybe a different pull request) module_naming_convention (since that's what lead us to add _SUITE to the default atom_naming_convention, IIRC).

elbrujohalcon commented 3 years ago

Yeah… I think that the ideal scenario would have atom_naming_convention only applying over atoms that are used as atoms, and leaving function_naming_convention and module_naming_convention for the other stuff.

paulo-ferraz-oliveira commented 3 years ago

But in the case I describe I'm not sure ktn is sending back enough elements for that analysis. Might have to pick this up later for further digging.

elbrujohalcon commented 3 years ago

It's a pure semantic discussion, but… In your scenario, I would say that atoms are used as atoms. What you're using as a function call are the variables ;)