-
Notifications
You must be signed in to change notification settings - Fork 171
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
Lint for more than one positional boolean argument #1638
Comments
Interesting idea! Another possible refinement to setState(false); feels better than: setState('alert', false); and certainly better than: setFooBar(true, false); Curious if @munificent has any experiences to share? |
I've never used the |
Thanks for chiming in Bob!
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? |
@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. |
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. |
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! |
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. |
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 |
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 |
I have this enabled and have to use @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 @pq Is there still plans on improving this linter to don't complain for single-parameter functions? |
I opened #4349 as just mentioned above. |
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:
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.The text was updated successfully, but these errors were encountered: