Sort public methods before private methods #3799

zellidev0 · 2022-10-28T15:48:22Z

New linter rule

I created a new linter rule which, when enabled, makes public methods appear before private methods.

My intention was to have a first step in more concise code.
Public methods should be at the top because they define the interface to the class.

Private methods are (most of the times) less important and therefore they should come below public methods.

Link to Issue

google-cla · 2022-10-28T15:48:26Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

lib/src/rules/sort_public_methods_before_private_methods.dart

bwilkerson · 2022-10-28T15:59:33Z

lib/src/rules/sort_public_methods_before_private_methods.dart

+class SortPublicMethodsBeforePrivateMethods extends LintRule {
+  SortPublicMethodsBeforePrivateMethods()
+      : super(
+            name: 'sort_public_methods_before_private_methods',


As written, this lint applies to all members, not just methods, so the name should reflect that by using 'member' in place of 'method'.

We might also want to shorten the name a bit. Perhaps something like sort_private_members_last.

Could you please elaborate why this also applies for other members?
As I understand it in line 66 if (member is MethodDeclaration) { this only applies for method declarations and not for other declarations, right?.
But I may have also missed something, because this is my first attempt to writing a lint rule.

For example this piece gets not complained about.

class A { final _y = 0; final x = 0; }

Regarding the naming I like the idea to shorten the name.
If this only applies to methods I would suggest sort_private_methods_last, when this applies to all members your suggestion looks good.

I will change the name when we decided if this only applies to methods or all members.

You're right, that comment was outdated. I hadn't noticed the test for method declarations on the first read through, so when I went back to start commenting I had the wrong understanding, and forgot to update the comment when I did see the test. Sorry for the noise.

I renamed the rule to sort_private_methods_last.

bwilkerson · 2022-10-28T15:59:43Z

lib/src/rules/sort_public_methods_before_private_methods.dart

+  _Visitor(this.rule);
+
+  void check(NodeList<ClassMember> members) {
+    bool privateMethodIsBeforePublicMethod = false;


I believe that what this variable actually represents is whether we have found a private member, not whether there is a public member after the private member. Consider changing the name to reflect that, perhaps something like foundPrivateMember.

You are right. The name foundPrivateMember reflects that better, so I'm changing it to that one.

bwilkerson · 2022-10-28T16:04:57Z

lib/src/rules/sort_public_methods_before_private_methods.dart

+      if (member is MethodDeclaration) {
+        if (privateMethodIsBeforePublicMethod &&
+            !_isPrivateName(member: member)) {
+          rule.reportLint(member.returnType);


Do we want to report this lint for every public member after a private member, or just the first?

Personally, I suspect that it's sufficient to report it for a single member.

Do we want to report this lint for public members or for private members? (That is, which does this lint consider to be in the wrong place?) The original name implies that the private members are in the right place and that it's the public members that need to be moved. The alternative I suggested (which might not be right) implies that the public members are right and that it's the private members that need to be moved.

As far as I know other rules report a lint suggesting for every occurrence.
E.g. the rule require_trailing_commas tells me all the places where a comma is missing.
So for being consistent over multiple rules, I would change it to display all the occurrences simultaneously.
Also e.g. in Android Studio/IntelliJ the shortcut that moves the cursor to the next error would be faster if all the occurrences are mentioned at the same time, in comparison to requiring the analyzer to run again and then mention the next occurrence.

So if you are fine with that, I would try to implement to show all the occurrences simultaneously.

I'm open for suggestions if this lint should report for private or public members.
Personally I would not prefer one over the other, because when the lint is resolved, the result is the same.
What would you suggest?

Regarding the report of multiple occurrences at one, this should already work.
I added a test to confirm that test_multiple_cases_are_reported_simultaneously.

As far as I know other rules report a lint suggesting for every occurrence.

Not quite, though I can see why it would seem that way.

The criteria we use is that every issue should have a single diagnostic. If there's a missing comma in one invocation, that's one issue and should have one diagnostic. If there's a missing comma in a second invocation, then that's a separate issue and should also have a diagnostic. Two separate issues, so two diagnostics.

But in this case it seems to me that the fact that the methods aren't sorted correctly is a single issue. For example, the code

class C { void m1() {} void _m2() {} void m3() {} void m4() {} void m5() {} void _m6() {} }

Looks to me like someone added a private method (m2) in the wrong place. But if the lint reports that m3, m4, and m5 are in the wrong place, that seems like noise to me. I'd much rather be told once that the methods aren't sorted correctly and then run "Sort Members" (or some other fix) to have it fix all the problems in this container.

It's possible that I'm the only one that feels that way.

I'm open for suggestions if this lint should report for private or public members.

I don't know which is better. I believe that it depends on the failure mode that most users experience, which is data I don't have. I wondered whether you had an informed decision, and it sounds like you don't. It might not matter too much, but I tend to think about such things.

I know understand what you mean and I think this is a better solution than linting every occurrence.
The linter should complain once per file to move the private methods below the public ones and a quick fix that does that would be a good solution.
I will try to implement that.

A quick fix for that is not implemented in the linter package right?
I guess this is implemented in the dart sdk which cannot simply be extended by a merge request?
Or am I wrong?

I'd hold off on doing more implementation work until we have gotten more feedback on the questions in the issue. We're being cautious about adding new stylistic lints and would like to gauge the reactions of the community before merging this PR.

The linter should complain once per file to move the private methods ...

I suspect we'll end up with one lint per class, enum, etc.

A quick fix for that is not implemented in the linter package right?
I guess this is implemented in the dart sdk which cannot simply be extended by a merge request?

Yes, quick fixes are implemented in the analysis_server package. There's documentation for how to do that (https://github.com/dart-lang/sdk/blob/main/pkg/analysis_server/doc/implementation/quick_fix.md).

You can create a PR against the sdk repo and it will automatically converted into a Gerrit CL. If you look at the workflow steps in GitHub there should be a link to the CL. You won't have permissions to do much with the CL, but you can push new branches to update your PR and they'll also automatically get copied across.

But I'd also hold off on implementing a fix for a couple of reasons:

We already have a "Sort Members" function that could act as a fix (although it does more that might be expected from a fix for this lint). It isn't clear that we want to support yet another sorting feature.

Once this PR is merged (assuming it is) it can take a week or more for those changes to roll into the sdk, and you won't be able to test the fix until the lint is in the sdk.

Alright, thanks for the information.

If the community likes this rule I would also try to create a PR in the sdk (assuming this one is merged).
I'm also looking forward to add more rules, if this is something the community would appreciate.

I would really like rules that sort members etc. like described in the Flutter style guide.
So that in the end all of the mentioned rules for ordering can be automatically applied to files (if wished).

But I will wait some time and come back to this PR when more from the community have seen my proposal.
Thanks a lot for the help and feedback.

…s and extensions as well. - Added tests for enums, mixins and extensions - renamed the variable `privateMethodIsBeforePublicMethod` to `foundPrivateMethod` to better reflect what it does

…est that multiple occurrences are reported simultaneously

…_private_methods_last

…tic and lint only once at the class level no matter the amount of occurrences

…ods inside of other methods

zellidev0 · 2022-10-28T22:23:05Z

@bwilkerson I also added a check so that static methods are ignored in this rule.
I think there should be a separate rule that handles the case where static methods in a class should be defined.
Like mentioned in the Flutter style guide.

bwilkerson · 2022-10-28T23:29:58Z

Like mentioned in the Flutter style guide.

I'll just mention that with very few exceptions, we don't support lints related to the Flutter style guide. Those are styles established for working in the flutter repository. Instead we support lints related to the Dart style guide. These are the guidelines recommended for Dart and Flutter users (and Dart developers). The Dart style guide specifies the sort order for directives, but doesn't specify any sort order for declarations.

zellidev0 · 2023-01-07T17:47:52Z

I will close this as linter rules related to the flutter style guide are not supported.

zellidev0 added 2 commits October 28, 2022 17:34

Added new rule to sort public methods before private methods in classes

084e886

format

a5cca67

bwilkerson reviewed Oct 28, 2022

View reviewed changes

zellidev0 added 7 commits October 28, 2022 21:54

apply rule sort_public_methods_before_private_methods to enums, mixin…

5689969

…s and extensions as well. - Added tests for enums, mixins and extensions - renamed the variable `privateMethodIsBeforePublicMethod` to `foundPrivateMethod` to better reflect what it does

format

ac6cbb3

added test to confirm in sort_public_methods_before_private_methods_t…

34bdd31

…est that multiple occurrences are reported simultaneously

rename linter rule sort_public_methods_before_private_methods to sort…

553a1de

…_private_methods_last

format

0ff0005

made sort_private_methods_last only apply to methods that are not sta…

8e1521f

…tic and lint only once at the class level no matter the amount of occurrences

added test that rule sort_private_methods_last does not apply to meth…

4b5cf19

…ods inside of other methods

zellidev0 closed this Jan 7, 2023

zellidev0 deleted the sort_public_methods_before_private_methods branch January 7, 2023 17:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sort public methods before private methods #3799

Sort public methods before private methods #3799

zellidev0 commented Oct 28, 2022

google-cla bot commented Oct 28, 2022

bwilkerson Oct 28, 2022

zellidev0 Oct 28, 2022

zellidev0 Oct 28, 2022

zellidev0 Oct 28, 2022

bwilkerson Oct 28, 2022

zellidev0 Oct 28, 2022

bwilkerson Oct 28, 2022

zellidev0 Oct 28, 2022 •

edited

Loading

bwilkerson Oct 28, 2022

zellidev0 Oct 28, 2022 •

edited

Loading

zellidev0 Oct 28, 2022

zellidev0 Oct 28, 2022

bwilkerson Oct 28, 2022

zellidev0 Oct 28, 2022

zellidev0 Oct 28, 2022

bwilkerson Oct 28, 2022

zellidev0 Oct 28, 2022 •

edited

Loading

zellidev0 commented Oct 28, 2022

bwilkerson commented Oct 28, 2022

zellidev0 commented Jan 7, 2023

Sort public methods before private methods #3799

Sort public methods before private methods #3799

Conversation

zellidev0 commented Oct 28, 2022

New linter rule

google-cla bot commented Oct 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zellidev0 Oct 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zellidev0 Oct 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zellidev0 Oct 28, 2022 • edited Loading

Choose a reason for hiding this comment

zellidev0 commented Oct 28, 2022

bwilkerson commented Oct 28, 2022

zellidev0 commented Jan 7, 2023

zellidev0 Oct 28, 2022 •

edited

Loading

zellidev0 Oct 28, 2022 •

edited

Loading

zellidev0 Oct 28, 2022 •

edited

Loading