dart-lang / linter

Linter for Dart.
https://dart.dev/tools/linter-rules
BSD 3-Clause "New" or "Revised" License
629 stars 170 forks source link

combinators_ordering: should be case INSENSITIVE! #3866

Open kevmoo opened 1 year ago

kevmoo commented 1 year ago

It's weird to see an ordering of SomeClass, aFunction

CC @srawlins

srawlins commented 1 year ago

CC @a14n who implemented combinators_ordering, and @bwilkerson who reviewed it.

@pq Can surveyor tell us what is typical? We can also look in SDK's pkg/front_end and I think pkg/_fe_analyzer_shared, where show is very common. I could poke around google3, but maybe not soon.

srawlins commented 1 year ago

To expand on the summary, since show-able and hide-able elements exclusively begin with [a-z] or [A-Z], is it better to keep the ordering case-sensitive, effectively creating two alphabetized lists of elements? Or is it better to make the ordering case-insensitive, which may make it easier to scan?

pq commented 1 year ago

@pq Can surveyor tell us what is typical?

Yeah. You should be able to gather statistics with a surveyor visitor given a good corpus.

lrhn commented 1 year ago

Either choice is trivial for a tool to check or enforce. The question is what is best for manual editing and reading.

Personally, I think I'd prefer case sensitive. Types and methods are separate enough, that I'd like to see them separately. I think it's more readable. If I look at such a list, or add to it, I already know whether I'm looking for a type or a function/variable. By keeping those separate, I only need to look through a smaller list.

I'd actually put lower-case names first, even if they ASCII-ordering-wise are after. It just feels "more right" to have methods before types. (Definitely a gut feeling thing, no hard reasons.)

Caveat: I write very few show/hide clauses, and if I do, it's rarely more than a handful of names, so this is not based on a lot of experience.

When adding to a show/hide list, I think I'd prefer to just append names and have the tool sort the list for me, than have to care where I put a new name. In that case, it also doesn't matter which ordering the tool prefers.

The case of having to add a name manually, to a long list, while keeping the list sorted correctly, ... is probably still slightly easier if the list ordering is case sensitive. I only have to look through half as many entries (well, probably more than half - if the balance is skewed, a new name is also more likely to be in the already larger part). The failure mode would be when you add the first lower/upper-case name to a long list of the other case, and you forget that it should go first/last instead of somewhere in the middle.

If that is something you do often, I think you'll quickly learn it. If not, you should have a tool to help you anyway. This is not a lint I'd want to use or enforce without an IDE fix.

So, lower-case before upper-case is my vote :)

since show-able and hide-able elements exclusively begin with [a-z] or [A-Z]

Nit: They can begin with $ too. Usually don't. I'd put those with the group of their second letter, probably first in that group (or by the second letter, and a second character of [0-9] or _ is treated as lower-case).