Abion47 / darq

A port of .NET's LINQ IEnumerable functions to Dart.
MIT License
85 stars 9 forks source link

Enumerable instance methods starting with upper case #1

Closed jporsay closed 5 years ago

jporsay commented 5 years ago

Love the idea for the library, but it's a bit shocking to see instance methods starting in upper case since that is not the preferred way specified by Effective Dart: Style.

Would you be OK with lower-casing them (or accepting a pull request that does that)?

knopp commented 5 years ago

Same thing I noticed immediately. Dart is very rigid about about code style and as the result, most packages are very consistent. This one breaks the consistency.

andrewackerman commented 5 years ago

@jporsay @knopp

Enumerable inherits from Iterable, which already has a number of methods that are named the same (such as join and where) but with a different method signature than what the Enumerable method would require. Since Dart doesn't support method overloading or shadowing, this was the only way to port the methods over from their .NET equivalents.

An alternative would be to go through and rename the methods to something that doesn't conflict with the existing Iterable methods, but then it wouldn't be consistent with the original LINQ methods and, as such, people expecting a method might assume it doesn't exist even though it does (just under a different name).

I decided to go with the CamelCase name option and see which compromise people felt more passionately about. If the CamelCase is that big of a deal, I'll explore the other option.

atresnjo commented 5 years ago

@jporsay @knopp

Enumerable inherits from Iterable, which already has a number of methods that are named the same (such as join and where) but with a different method signature than what the Enumerable method would require. Since Dart doesn't support method overloading or shadowing, this was the only way to port the methods over from their .NET equivalents.

An alternative would be to go through and rename the methods to something that doesn't conflict with the existing Iterable methods, but then it wouldn't be consistent with the original LINQ methods and, as such, people expecting a method might assume it doesn't exist even though it does (just under a different name).

I decided to go with the CamelCase name option and see which compromise people felt more passionately about. If the CamelCase is that big of a deal, I'll explore the other option.

Maybe do it like https://github.com/jackmott/LinqFaster ?

His method names just have an extra character.

Love the library, good work!

knopp commented 5 years ago

I also think prefix or suffix would be a more palatable solution in this case. This looks a bit like an eyesore within the ecosystem, which is shame, as I really do like the idea.

andrewackerman commented 5 years ago

@atresnjo @knopp

That might work. How about an e prefix (for Enumerable), like eSelect and eJoin? Would that be distinct enough, or would that be confusing for people who use the e prefix to mark enums?

knopp commented 5 years ago

I think e is fine, or even q (as for the darq). I was even playing with something like e.select and e.where, but that doesn't work well with dartfmt. Maybe suffix could also be worth considering, but prefix does have the benefit of autocompleter listing all relevant functions.

andrewackerman commented 5 years ago

@knopp

Which do you think looks more aethetically pleasing:

eSelect
eJoin
eWhere
eMax
eMin
eGroupBy
eAggregate
qSelect
qJoin
qWhere
qMax
qMin
qGroupBy
qAggregate
selectE
joinE
whereE
maxE
minE
groupByE
aggregateE
selectQ
joinQ
whereQ
maxQ
minQ
groupByQ
aggregateQ

Personally I think the suffix does match better with Dart code styles as the prefix satisfies dartfmt but still looks like CamelCase. I'm leaning toward using E as well, as it can stand for Enumerable but could also mean Extended as in "extending the functionality of Iterable" (which I plan to move toward once Dart supports extension methods).

andrewackerman commented 5 years ago

I've implemented the "suffix E" approach in https://github.com/andrewackerman/darq/commit/81ce49a4c0e26de658b1dad52cf9128256d92d44 and pushing the change as version 0.2.0 (among other things). If everything looks good to everyone, I'll close the issue.