passsy / dart-lint

An opinionated, community-driven set of lint rules for Dart and Flutter projects. Like pedantic but stricter
Apache License 2.0
277 stars 82 forks source link

sort_constructors_first is too opinionated #1

Closed passsy closed 4 years ago

passsy commented 5 years ago

Until now I haven't found a single project using this rule. Better remove it.

Juneezee commented 3 years ago

@passsy The flutter repo is using this rule

Reference: https://github.com/flutter/flutter/blob/de56c6ad725d58e98772e37125c8762ec2f643c3/analysis_options.yaml#L188

rydmike commented 3 years ago

Yes @Juneezee they are indeed using it, it is however not always so nice, matter of opinion of course. Here is my reasoning around this rule.

I do like the default unnamed constructor first, and that is ON via another rule, but after it I want to see it followed by its properties, and then after this other named constructors and factories. This rule gets in the way of that order and forces you to put (often final) constructor properties after all the named constructors and factories, making them tedious to find and disconnected from where I want to see, read and maintain them. This is especially the case if there are many constructors and factories and they have a lot of parameters.

I also disabled this rules in my personal custom rules and order things as described above, which apart from the default constructor properties coming right after the constructor, is where it I deviate from the rule, but otherwise I do put constructors first.

Like you said, the entire Flutter repo does use this rule, but take a look at eg ThemeData and many other classes with a long list of properties and different constructors and factories, you have to really scroll and search for the properties with their doc comments. and adding new properties gets tedious with the long distance between its declaration and usage in the default constructor.

I think @passsy made the right call here, of course that is opinionated too, but many lint rules are 😄

jeroen-meijer commented 3 years ago

Just my two cents -- I really like this rule. 😄

Reason being is that, as a user of some Dart API, package or components, I find that how to interact with a given class is more useful and interesting to me than what the class contains, especially since a constructor does not always fully align with the fields that are inside it (like, for example, Size.square(double dimension)).

If I want to dive into the specific fields a class contains or what its inner workings look like, I can scroll over to the fields and methods.

Additionally, for code that isn't null-safe yet, I find it useful to immediately see what arguments can and cannot be null, and see other assertions unrelated to null.

... but take a look at eg ThemeData and many other classes with a long list of properties and different constructors and factories, you have to really scroll and search for the properties with their doc comments. and adding new properties gets tedious with the long distance between its declaration and usage in the default constructor.

Though I understand the irritation here, this is a minor nuisance to me and doesn't weigh against the advantages this rule provides for readability when it comes to API consumption. Using an IDE, it's easy to find any field or constructor (in VS Code, the default binding is ⌘+Shift+O/Ctrl+Shift+O), so I don't see this as an issue. 👍🏻