Lint for more than one positional boolean argument

[Anrock]

I would like to see a linter rule similar to avoid_positional_boolean_parameters but it should trigger only if there are more than one positional boolean parameter.

Suggested name: avoid_multiple_positional_boolean_parameters

---

From my experience, every time avoid_positional_boolean_parameters was triggered I had to jump through additional hoops to make linter happy (YMMV). Ignoring this rule was actually more easier and better solution. Most times it was a function with single positional boolean parameter, like:

setReadOnlyMode(bool mode);
// or
class SomeBooleanStateChangedAction { 
  final bool state; 
  SomeBooleanStateChangedAction(this.state);
}

So from this we can conclude that positional boolean argument are not always ambigious, therefore they're aren't always bad.

However I think that two or more positional boolean arguments are always bad because function name won't give you enought context at call site.

So for me, avoid_positional_boolean_parameters is too strict and requires ignoring false positives.

avoid_multiple_positional_boolean_parameters should be a more bullet-proof rule without false positive cases, if we assume that there is no valid "good" case that would use multiple positional boolean arguments. And I can't think of any such case.

[pq]

Interesting idea! Another possible refinement to avoid_positional_boolean_parameters would be to allow single argument lists but disallow all others. For example:

setState(false);

feels better than:

setState('alert', false);

and certainly better than:

setFooBar(true, false);

Curious if @munificent has any experiences to share?

[munificent]

I've never used the avoid_positional_boolean_parameters lint myself, but I wouldn't be surprised that there are exceptions. There's a reason it's not a hard rule. Personally, I think it's probably just simpler to not lint this case at all than to try to add increasingly subtle lints to trace that boundary.

[pq]

Thanks for chiming in Bob!

Personally, I think it's probably just simpler to not lint this case at all than to try to add increasingly subtle lints to trace that boundary.

In general this is the attitude I take and I strongly prefer simplicity to nuance in lints.

@Anrock: is this something that's tripped you up a lot? Would a refinement that allows single-argument methods w/ a positional boolean cover your common cases?

[Anrock]

@pq tripped me a couple of times, so in the end I've disabled it and rely on code review. Proposed refinement will cover some cases, but not all. At least it sounds like bulletproof lint without false positives, so I can enable it and remove some burden from reviewers. Better than nothing.

[srawlins]

I generally agree with @pq and @munificent that it's impossible to fine tune some lints to the point that they get zero complaints or push back or false positives.

However, in this case, I strongly suspect that going from "complain on 1+ positional booleans" to "complain on 2+ positional booleans" would move this lint from "useful to a few die hards" to "useful to the masses." I would not recommend forking into a new lint, because of the maintenance burden and cognitive burden of users looking at two rules and choosing.

I'm a big +1 👍 to just bumping the minimum from 1 to 2.

[pq]

Unless there's push-back, I'm in favor of bumping to "complain on 2+ positional booleans" and will likely go for it.

Do chime in if any of you hold reservations!

[bwilkerson]

Do we have any idea about how many users enable this rule? If there are enough users then I would be hesitant to change the semantics of the existing rule.

[pq]

Thanks @bwilkerson . I was just looking at this.

Greping through a local cache of just over 2000 analysis options files culled from over 4000 packages downloaded from pub, I'm only seeing avoid_positional_boolean_parameters enabled 189 times. (Interestingly it's explicitly commented out 126 times.)

[dustyholmes-wf]

I am also interested in an adjustment to this lint. It is something my team would like to enable but I've found boolean setting functions are problematic.

import 'dart:async';

import 'package:meta/meta.dart';

void main() async {
  final controller = StreamController<bool>.broadcast();
  final receiver = BoolReceiver();

  // The only setup that I've thought of that satisfies linters and our best practices. It introduces a closure,
  // which I think makes it more difficult to read at first sight.
  controller.stream.listen((b) => receiver.setValueByName(value: b));

  // This is slightly less verbose, but still requires a closure to make a listener.
  // Our best practices for module api development recommend avoid_setters_without_getters so that we can use 
  // simpler listen directives.
  controller.stream.listen((b) => receiver.value = b);

  // This is my personal favorite, it is terse and clear. 
  // avoid_positional_boolean_parameters effectively outlaws this style.
  controller.stream.listen(receiver.setValue);

  await controller.close();
}

class BoolReceiver {
  bool _value;

  // Contradicts avoid_positional_boolean_parameters
  void setValue(bool value) => _value = value;

  // Normally would just be called setValue, but I wanted to show 3 examples here. 
  void setValueByName({@required bool value}) => _value = value;

  // Contradicts avoid_setters_without_getters
  set value(bool v) => _value = v;
}

I actually really like the idea that we only allow single parameter functions which take exactly one positional boolean. Any function that takes more than a single boolean parameter is almost certainly a good target for this lint

[mateusfccp]

I have this enabled and have to use // ignore: avoid_positional_boolean_parameters sometimes for single-arguments functions. One common case is this one (adapted from real code):

@freezed
class Settings with _$Settings {
  const factory Settings({
    required double value,
    @JsonKey(name: 'typeAllocation', toJson: isChangeRecurrentToJson) required bool isChangeRecurrent,
  }) = _Settings;

  factory Settings.fromJson(Map<String, Object?> json) => _$SettingsFromJson(json);

  // ignore: avoid_positional_boolean_parameters
  static String isChangeRecurrentToJson(bool isChangeRecurrent) => isChangeRecurrent ? 'VAC' : 'VPD';
}

In this case I have no option, as we still don't have constant function literals, so I can't pass a function literal to the annotation.

Another common case is when passing functions to ValueChanged<bool> APIs on Flutter, like on Switch or Checkbox. Most times we pass a function literal, which does not trigger the linter. However, when the function is longer than a few lines, it's a good idea to extract it into a proper function, which triggers the linter.

---

@pq Is there still plans on improving this linter to don't complain for single-parameter functions?

[FMorschel]

Is there still plans on improving this linter to don't complain for single-parameter functions?

I opened #4349 as just mentioned above.