New Lint: avoid_using_api

[getBoolean]

Closes #81

This lint has a lot of edge cases. Tests are included to verify basic functionality.

Packages Added:

• glob: Determines which files an entry should apply to (better than Regex for paths)
• yaml: To convert YamlMap to Map for the list of entries

Usage

External code can be "deprecated" when there is a better option available:

custom_lint:
  rules:
    - avoid_using_api:
      severity: info
      entries:
        - class_name: Future
          identifier: wait
          source: dart:async
          reason: "Future.wait should be avoided because it loses type safety for the results. Use a Record's `wait` method instead."
          severity: warning

Result:

void main() async {
  await Future.wait([...]); // LINT
  await (...,).wait; // OK
}

Advanced Usage

Each entry also has includes and excludes parameters. These paths utilize Glob patterns to determine if a lint entry should be applied to a file.

For example, a lint to prevent usage of a domain-only package outside of the domain folder:

custom_lint:
  rules:
    - avoid_using_api:
      severity: info
      entries:
        - source: package:modddels
          excludes:
            - "**/domain/**.dart"
          reason: "MODDDELS is only intended to be used in the domain layer."

[getBoolean]

I addressed most of the comments, the ones remaining are:

1. Rename parameter id to something that represents methods and variables, not sure what to name it.
2. Make source required. What should it do if source is not provided?
3. Rename lint. I'll add deprecate_external_api as an additional choice

[yurii-prykhodko-solid]

@getBoolean Apologies for a tardy response -- I see you've made quite a bit of progress!

1. Rename parameter id to something that represents methods and variables, not sure what to name it.

Maybe pattern? You are using glob matching under the hood, seems consistent.

2. Make source required. What should it do if source is not provided?

I'd say just ignore it, and provide ample warning in rule documentation & usage examples.

3. Rename lint. I'll add deprecate_external_api as an additional choice

Oh this one sparked quite a discussion :D

The consensus was to use the avoid_using_api name.

[getBoolean]

@yurii-prykhodko-solid glob pattern matching is only used for includes/excludes filepaths, not the id/class name. Is that something we want to change?

[yurii-prykhodko-solid]

@getBoolean Ah, confused the two. Then pattern is no good.

lexeme? May be too broad in meaning. symbol? Not really a common term in context of Dart lang.

And both of the above seem just as confusing as id to me. So I'd say we can leave it as is.

@solid-vovabeloded maybe you have some ideas?

[getBoolean]

@yurii-prykhodko-solid Where should I put the lint documentation?

[yurii-prykhodko-solid]

@getBoolean

• Rename parameter id to something that represents methods and variables, not sure what to name it.

Discussed this internally, the consensus was to use identifier, with no abbreviation. "Identifier" is the term Dart team uses for everything named -- functions, variables, classnames, which seems consistent with what the lint is expecting.

Where should I put the lint documentation?

That is an interesting question -- so far we've been maintaining this project as a drop-in solution, keeping configuration as minimal as it's possible, and therefore with little library user facing documentation.

I'd say we can leave this as an issue on this repo until we come up with a general approach of documenting our lints.

[getBoolean]

I think that's everything!

I do think documentation is needed, it will be very useful to know what all the lints are and what triggers them. Documentation for lints options can be included alongside that.

[getBoolean]

I hope that is everything.

[illia-romanenko]

Nice! Our CI still fails - it feels like we need to fix those issues before merging?

[getBoolean]

Not sure why its failing, I'll look into it when I get home from vacation in a week.

[getBoolean]

I've confirmed it working correctly on Windows and MacOS. It might be an issue somewhere in AvoidUsingApiLinter._matchesSource on Linux or CI causing the source check to fail.

[getBoolean]

Good thing I setup Windows/Linux dual boot already, made tracking down the issue a lot easier.

The issue was that node.sourceUrl would return the filepath on Linux for project files, while it was always the package path on Windows/MacOS. In the documentation, maybe we should specifically state that doing source: package:myapp will not work on Linux and should be avoided. I updated the lint_test code to move the banned code to another package.

I also found two edge cases while debugging and fixed them

• lint for method usage on variable of a banned class type (source was previously ignored for it)
• lint for variable declaration of a banned class type if the type was inferred.

[yurii-prykhodko-solid]

In the documentation, maybe we should specifically state that doing source: package:myapp will not work on Linux and should be avoided.

Which makes sense -- that's what 'Deprecated' is for.

Let's add this as a note in code documentation, I think the AvoidUsingApiEntryParameters docs are the right place.

[yurii-prykhodko-solid]

Let's add this as a note in code documentation, I think the AvoidUsingApiEntryParameters docs are the right place.

Done.

@getBoolean Let me know if you're happy with my changes, then we can finish this up.

[yurii-prykhodko-solid]

@getBoolean Looks like there's some more issues -- CI still in the red.

[getBoolean]

Before we merge, I want to investigate making _matchesSource take a non nullable parent source url, it was hiding a lint issue before.

@yurii-prykhodko-solid it looks like pub get needs to be run on those test packages in CI, is that possible?

[yurii-prykhodko-solid]

@yurii-prykhodko-solid it looks like pub get needs to be run on those test packages in CI, is that possible?

Sure -- have a stab at our CI file. A find -> exec or a find -> for should do the trick.

UPD: Happened to need this on a different project, CI now fetches dependencies for all pubspecs it can find.

[getBoolean]

Thanks, I think that is everything now