Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
nixxquality opened this issue Jul 31, 2021 · 11 comments
Milestone

Comments

@nixxquality
Copy link

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
Copy link
Member

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
Copy link
Author

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

@elbrujohalcon
Copy link
Member

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
Copy link
Member

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
Copy link
Author

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
Copy link
Member

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

@paulo-ferraz-oliveira
Copy link
Collaborator

paulo-ferraz-oliveira commented Jul 31, 2021

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:

  • {mod, fun, arity}
  • mod:fun/arity
  • Mod:Fun(...) where Fun is previously an atom
  • function declarations
  • ...

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
Copy link
Collaborator

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
Copy link
Member

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
Copy link
Collaborator

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
Copy link
Member

elbrujohalcon commented Aug 12, 2021

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 ;)

@elbrujohalcon elbrujohalcon changed the title atom_naming_convention is incompatible with Erlang's zlib module Do not apply atom_naming_convention over function names, only atoms used as atoms May 26, 2022
@elbrujohalcon elbrujohalcon added this to the 2.0.0 milestone May 26, 2022
@elbrujohalcon elbrujohalcon modified the milestones: 4.1.0, 4.0.0 Nov 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants