Open a14n opened 6 years ago
@bwilkerson @pq wdyt ?
Honestly, I have mixed feelings. On the one hand, this can be really useful and allow a much richer UX. But it's easy to abuse, and to end up with rules that are difficult to configure.
I worked on a similar product at a previous company, and we made the mistake of adding configuration support. We didn't have outside contributions, so we had no one to blame but ourselves, but we ended up with a product that users couldn't use until they had spent multiple hours going through the list of lints to decide whether they wanted to use the lint and if so how to configure it. The task was so daunting that many of our customers never realized the full potential value of the product.
So, yeah, it would be great to be able to specify the file header to a lint so that the quick fix could insert it when it's missing. On the other hand, I'm not convinced that it isn't better, for something like "include local variables in the lint" to just make them two separate lints and allow users to enable the functionality they want by choosing the lints they enable (something they have to do anyway).
I always thought we'd do this (see for example this old tracking issue: https://github.com/dart-lang/linter/issues/7) but as time has gone on I'm not as keen (for the reasons that Brian cites).
Very open to reconsidering though!
I agree configuration should be an exception. And for a file_header
lint I pointed above configuration is definitively mandatory.
@bwilkerson
But it's easy to abuse, and to end up with rules that are difficult to configure.
So the way to go as I see it is to introduce just a few super basic options to a linter rule which will not allow overconfiguring by design.
A good example is the Ruby linter which introduces a few basic options for each rule: Exclude
, Enabled
and EnforcedStyle
, and maybe 1 or 2 specific (but still basic) options depending on the rule.
Here's how it looks:
Layout/AccessModifierIndentation:
EnforcedStyle: outdent
IndentationWidth: 0
Layout/LineLength:
Exclude:
- config/**/*
Max: 80
Lint/EmptyBlock:
Exclude:
- spec/factories/**/*
Style/Lambda:
EnforcedStyle: literal
FWIW, when trying to enable implementation_imports
in analyzer I found myself really missing this. What I'd like is to be able to say _fe_analyzer
is privileged (or friends or similar) since really src
imports in this case are expected.
It'd be interesting to explore places that (judicious) configuration would really open up and weigh them against the downsides of the added complexity.
Input welcome!
Is the problem that implementation_imports
needs to be configurable because sometimes users validly need to reach into the src
directory, or is the problem that _fe_analyzer_shared
(and a few other packages in the SDK) isn't following the established conventions, and hence a lint to enforce that convention can't be used? Personally, I think it's the latter and that making the lint configurable would be bad for most of our users.
Ah. Excellent point. If _fe_analyzer_shared
were structured more for code sharing this would be a non-issue.
/fyi @stereotype441 @johnniwinther
Yes, but it's structured the way it is because we're explicitly not making any guarantees about the API that it provides. Packages outside the SDK should never depend on it or import it.
Understood. But isn't that what the _
is for?
In any event, probably more fuss than it's worth to get around a handful of //ignore
s...
I'm not aware of any enforcement of the convention (which might only exist for this one package), but yes, it's prefixed with an underscore to attempt to communicate that it's private.
+1 to what Brian said about _fe_analyzer_shared
. It's not so much that it's not structured for code sharing, it's that it's structured for use only by the analyzer and the front end. We really don't want to take on the maintenance burden of having other packages start to depend on it and then file bugs when we change its API.
There are other approaches we could use instead, I suppose. For example, _fe_analyzer_shared
could be a subdirectory of the analyzer's lib/src
directory, and then the front end could make use of it via import 'package:analyzer/src/_fe_analyzer_shared/...
. But that kind of seems worse than what we're doing today.
I agree with Phil that, at least in the case of _fe_analyzer_shared
a few // ignore
comments are a small price to pay compared to restructuring a bunch of stuff.
As pointed in #4528, the behaviour of "avoid_positional_boolean_parameters
is only triggered by public methods" is not expected but was designed that way.
I do believe that there could be one more option inside the analyzer
or similar where we could define project-wise if some lint will trigger for private members or not. Or even, there could be an ignore-for-file
tag or other ignore name that would allow us to set that for a file.
Something similar to what we already have today with:
analyzer:
plugins:
- dart_code_metrics
language:
strict-casts: true
exclude:
- "**.g.dart"
errors:
missing_required_param: error
# Added new option:
ignore_for_private_members:
- avoid_positional_boolean_parameters
Or in the ignore something like:
// ignore-for-private-members-in-file: avoid_positional_boolean_parameters
That could also, at least help with https://github.com/dart-lang/sdk/issues/54374 main idea for example.
It would be useful and nice to be able to customize some lints with parameter/configuration.
For instance: