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

Naming/BlockParameterName + stdlib methods like sort #453

Open
petr-fischer opened this issue Jan 26, 2024 · 4 comments
Open

Naming/BlockParameterName + stdlib methods like sort #453

petr-fischer opened this issue Jan 26, 2024 · 4 comments

Comments

@petr-fischer
Copy link

Hello - today, ameba complains for usage one-letter block parameter names in the stdlib sort method usage.

src/xxx.cr:58:91
[C] Naming/BlockParameterName: Disallowed block parameter name found
> ... emails.sort { |a, b| b.time <=> a.time }

But even in the Crystal docs, there is example with exact that short block parameter names:

a = [3, 1, 2]
b = a.sort { |a, b| b <=> a }
# ...

https://crystal-lang.org/api/1.11.2/Array.html#sort%28%26block%3AT%2CT-%3EU%29%3AArray%28T%29forallU-instance-method

How would you imagine more descriptive block parameter names when using the sort method? "a" and "b" are optimal.

Thanks!

@Sija
Copy link
Member

Sija commented Jan 26, 2024

We could add those to the exclusion list.

@petr-fischer
Copy link
Author

Yes please. And also: l, m, n and z :)

@straight-shoota
Copy link
Contributor

straight-shoota commented Jan 28, 2024

This rule is causing confusion and I don't think it's very helpful. Can it be disabled by default?
Short names are not inherently bad, particularly in short blocks.
The main point against single-character names is that variable names should ideally be descriptive. This is more relevant when used in bigger scopes. But that's a bit mute when declaration and single use are just a couple of characters apart. Then it can be more like a unique identifier without much embedded contextual information.

Related: https://forum.crystal-lang.org/t/ameba-warning-i-dont-understand/6377

@Sija
Copy link
Member

Sija commented Apr 3, 2024

Disabling it by default is a viable option, let's do that.

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