solid-software / solid_lints

🟧 Lints for Dart and Flutter based on software industry standards and best practices.
Other
36 stars 17 forks source link

avoid_using_api: Lint Default Constructor Only #126

Open getBoolean opened 4 months ago

getBoolean commented 4 months ago

While using the lint I noticed I missed an edge case, it is not possible to lint only the default constructor. it will always lint all usages of the class

Potential usage:

custom_lint:
  rules:
    - avoid_using_api:
      entries:
        # Add parentheses to lint only default constructor, use `identifier` for named constructors
        - class_name: Border()
          source: package:flutter
          reason: Use `BorderDirectional` instead.

Not sure if this is the best way to implement it, and I could see it being confusing if named constructors (ie class_name: Border.all()) didn't also work. Thoughts?

I can implement this.

getBoolean commented 4 months ago

I also want to explore linting constructor parameters

illia-romanenko commented 4 months ago

Maybe something like that?

custom_lint:
  rules:
    - avoid_using_api:
      entries:
        # Add parentheses to lint only default constructor, use `identifier` for named constructors
        - class_name: Border
          method: ()
          source: package:flutter
          reason: Use `BorderDirectional` instead.

and then

custom_lint:
  rules:
    - avoid_using_api:
      entries:
        # Add parentheses to lint only default constructor, use `identifier` for named constructors
        - class_name: Border
          method: all()
          source: package:flutter
          reason: Use `BorderDirectional` instead.
getBoolean commented 4 months ago

In your example, is method a new option in addition to identifier? Or did you mean to use identifier?

I think that would work better.

for linting method parameters, I was thinking we could do something like this:

custom_lint:
  rules:
    - avoid_using_api:
      entries:
        - class_name: Border
          identifier: (left)
          source: package:flutter
          reason: Use `BorderDirectional(start)` instead.
illia-romanenko commented 4 months ago

Yes - that should be it. Not sure what left means here but just () should be the best.

getBoolean commented 4 months ago

left was a constructor parameter in the default constructor. Seems a bit confusing putting it in parentheses, so maybe a separate input field parameter would make it clear

The intent for my second comment was to only lint usage of a specific parameter in a constructor (or method)

final border = const Border(left: 4.0); // LINT
final border2 = const Border(top: 4.0); // Okay
custom_lint:
  rules:
    - avoid_using_api:
      entries:
        - class_name: Border
          identifier: ()
          parameter: left
          source: package:flutter
          reason: Use `BorderDirectional(start)` instead.
illia-romanenko commented 4 months ago

Oh that's interesting! What if multiple parameters? It could be named named_parameter(s) as they are only variable, wdyt?

getBoolean commented 4 months ago

I'm not sure how much benefit having multiple named parameters in a lint entry would provide. In my opinion, it's best if the lint reason is as specific as possible and should say what to use as a replacement. Linting multiple named parameters (while convenient) would require a more general lint reason, it wouldn't be able to say exactly what should be changed.

I'm in favor of it being named named_parameter, that should make it clear it doesn't work for positional parameters.

illia-romanenko commented 4 months ago

I think it's settled then, we can return to multiple named parameters if the need arises.

getBoolean commented 4 months ago

Instead of empty parentheses, I think it would be better to use the new keyword

custom_lint:
  rules:
    - avoid_using_api:
      entries:
        - class_name: Border
          identifier: new
          named_parameter: left
          source: package:flutter
          reason: Use `BorderDirectional()` instead.
illia-romanenko commented 3 months ago

Since new is usually omitted - it feels like empty parentheses would be more understandable.