rrrene / credo

A static code analysis tool for the Elixir language with a focus on code consistency and teaching.
http://credo-ci.org/
MIT License
4.94k stars 419 forks source link

Add duplicated alias check #1072

Closed viniciusmuller closed 9 months ago

viniciusmuller commented 1 year ago

This PR adds a check for detecting duplicated aliases in files.

These can end up being merged after solving merge conflicts if we have modules that use many aliases and are not that easy to spot.

Using this check we can check for duplicated alias right in CI and prevent code like this from being merged.

Currently this PR don't target multi-aliases, but I think it would be useful to target it as well. If there's interest let me know and I'll add this

rrrene commented 11 months ago

Hi, sorry for commenting so late.

I am not really convinced that this needs to be in Credo itself. Is this really such a common problem as described in the check's explanation?

I am not saying that there could not be a version of this accepted at some point in the future, but at least the use case of peer review and the provided test cases do not really convince me.

rrrene commented 10 months ago

@viniciusmuller So, I slept on this for a couple of weeks and without any more input, I am sorry but deciding against this.

Credo's runtime does not get shorter by introducing more default-enabled checks that try to prevent very team-specific issues.

Nevertheless, this is a very good example for a custom check. 👍

If it is okay with you, I would like to use this as a code example for custom checks in the future?

viniciusmuller commented 10 months ago

Hey, sorry for the delay as well, I agree with you, this is very specific and I managed to implement it easily with a custom check

It's also a bit more complicated than it looks if one wants to cover all those cases and make it a default, because having duplicates alias can theoretically do something if there are nested modules with the same name (which shouldn't be a thing, but who knows 😆)

Feel free to adapt this implementation as a custom check example 😄

paulo-ferraz-oliveira commented 9 months ago

This seems like a job for the compiler, I'd say (unless there's a quirk when doing it); I've also found myself in the same situation as described, with pull requests making it possible for the code to present duplicate aliases. In Erlang, as an example, if you try to export a function twice you get something like : Warning: function Fun/Arity already exported, but this is a compiler warning, not a linter warning.

rrrene commented 9 months ago

This seems like a job for the compiler, I'd say

@paulo-ferraz-oliveira completely agree.

Just to be clear: I merged the PR and then moved the check to the docs, to credit @viniciusmuller with the contribution. ✌️