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

avoid_positional_boolean_parameters is only triggered by public methods #4528

Open
pitazzo opened this issue Jul 4, 2023 · 9 comments
Open
Labels
false-negative P4 type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)

Comments

@pitazzo
Copy link

pitazzo commented Jul 4, 2023

Hi there,
I've just noticed that the avoid_positional_boolean_parameters rule only is checked for the public methods of a given class. I don't know if this is a bug or a feature, but in my opinion it should work for both public and private methods, or at least, be configurable. Here is a minimal reproducible example:

class X {
  void bar() {
    foo(false);
    _faa(true);
  }

  // triggers the linter rule
  void foo(bool lele) {}

  // does not trigger the linter rule
  void _faa(bool lele) {}
}
@pitazzo pitazzo changed the title avoid_positional_boolean_parameters only works for public methods avoid_positional_boolean_parameters is only triggered by public methods Jul 4, 2023
@pitazzo pitazzo changed the title avoid_positional_boolean_parameters is only triggered by public methods avoid_positional_boolean_parameters is only triggered by public methods Jul 4, 2023
@lrhn
Copy link
Member

lrhn commented Jul 4, 2023

I'd personally prefer that it doesn't apply to members that are not part of a public API. Internal APIs may be written for briefness or simplicity over usability, and that's not a problem for anybody but the author themselves. If they're happy, there is no problem.

Public APIs also require everybody else to be happy too.

@pitazzo
Copy link
Author

pitazzo commented Jul 5, 2023

I see your point @lrhn, but even being a problem responsibility of the author of the library, I think it would be interesting to make the rule, at least, configurable. I myself, as the author of my own app, spent 2 hours yesterday debugging some boolean guards that I had passed in the wrong order. As an author, I think it is interesting that the linter warns of such problems, as it is easy to forget until problems arise.

@bwilkerson
Copy link
Member

The description doesn't make the intent clear, but the code, in multiple places, explicitly excludes private members, so I have to assume that it's intentional.

We don't support configuring lint rules.

The only alternative I can think of, though I don't really like it, would be to define an equivalent lint for private members and document the connection between the two.

@pq pq added the P4 label Jul 17, 2023
@srawlins srawlins added the type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) label Apr 3, 2024
@FMorschel
Copy link
Contributor

I do believe that this lint should trigger on private members as well. Here is a snippet of a comment that srawlins made in another issue

// ignore: is a great tool, and sends a strong signal, "I know this is unused, but I want to keep it."

According to what you said above:

We don't support configuring lint rules.

Since there are ignore, ignore_for_file, and even analyzer configs on analysis_options.yaml I don't think that is entirely true. Maybe there could be something similar to ignore to ignore private members, or even something similar inside here (analyzer config example):

analyzer:
  plugins:
    - dart_code_metrics
  language:
    strict-casts: true
    strict-inference: true
    strict-raw-types: true
  exclude:
    - "**/generated_plugin_registrant.dart"
    - "**.g.dart"
  errors:
    missing_required_param: error

In this example, I've added a plugin, configured the use of language lints, excluded paths where those lints will not trigger and changed the defining tone of a lint to error.

To my understanding, this is some kind of configuration similar to what was asked here.

@bwilkerson
Copy link
Member

We don't support configuring lint rules.

Since there are ignore, ignore_for_file, and even analyzer configs on analysis_options.yaml I don't think that is entirely true.

I don't think of those affordances as ways to configure a lint because they don't change the behavior of the lint, only where and how it will be reported. They apply equally well to any warning or error.

The kinds of things that fall under 'configuration' (the way I think about it) are things that change the way a specific lint works, such as the (implied) 'include private APIs' flag mentioned above.

@FMorschel
Copy link
Contributor

IMO this would be pretty similar to what exclude does in analyzer.

Say, we enable this lint to trigger on private APIs as well (which is why this issue was created from what I can gather, the way it is working now, it's unexpected), then if anyone doesn't want this lint to trigger on private APIs could simply add that to analysis_options.yaml.

@bwilkerson
Copy link
Member

The distinction in my head between the definition of a 'general affordance' and a 'configuration' probably isn't worth worrying about, other than to understand that there are some things that I'd consider to be a 'configuration' that you migiht not, and it's the things that I'd consider to be a 'configuration' that we don't currently support.

Unfortunately, something like "whether or not to lint on non-public APIs" is one of those things that we don't currently support.

@FMorschel
Copy link
Contributor

Do you believe that this could be discussed in a new issue? So that others can find it more easily and comment their opinions there as well.

One that would only request that configuration to be supported generally in some way?

Which repo would be the right one for that?

@bwilkerson
Copy link
Member

The issue already exists: #883.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
false-negative P4 type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)
Projects
None yet
Development

No branches or pull requests

6 participants