Skip to content

I wish the linter was extensible for internal-only rules #34145

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

Closed
matanlurey opened this issue Aug 14, 2018 · 8 comments
Closed

I wish the linter was extensible for internal-only rules #34145

matanlurey opened this issue Aug 14, 2018 · 8 comments
Labels
customer-google3 devexp-linter Issues with the analyzer's support for the linter package legacy-area-analyzer Use area-devexp instead.

Comments

@matanlurey
Copy link
Contributor

matanlurey commented Aug 14, 2018

i.e. checks that are too strict/contentious/specific for the rest of the world to have to deal with, or need more validation before we agree to start maintaining them for a larger set of users (i.e. every future Flutter user).

I'm guessing we could just manually add rules here:

... in the internal version (i.e. patch it). Is that desirable, though?

Another option is embracing the @srawlins "strict mode", where strict mode here would implicitly mean "for Dart at Google", and we could find ways to augment that mode.

As a side-effect, a customizable linter might also let the Flutter SDK folks (/cc @Hixie) write their own rules that don't need to live in dart-lang/linter - they will have more control and we will confuse users less (i.e. "Flutter style/Dart style" versus "Flutter SDK style").

@matanlurey matanlurey added devexp-linter Issues with the analyzer's support for the linter package customer-google3 labels Aug 14, 2018
@matanlurey
Copy link
Contributor Author

/cc @bwilkerson @pq for thoughts.

@bwilkerson
Copy link
Member

I'd be concerned about the possible conflicts that could result from patching, so that wouldn't be my favorite answer. @keertip might know of other issues related to patching.

Personally, I'd prefer to just have a separate plugin that gets loaded when running internally. Not only would that allow us to add internal-only lints, but would also allow us to add fixes for those lints, assists to ease internal coding tasks, internal-specific code completion, etc.

@matanlurey
Copy link
Contributor Author

@bwilkerson:

Personally, I'd prefer to just have a separate plugin that gets loaded when running internally

Do plugins get loaded for the CLI, as well? Currently all analysis checks outside of an IDE use the CLI. I was under the impression that was the main technical limitation (well, and that the analyzer team wasn't completely pleased with the plugin architecture today).

@bwilkerson
Copy link
Member

Do plugins get loaded for the CLI, as well?

No, but it's something that I think is important for us to support (and that this would help motivate) and wouldn't take much effort (on the order of days).

... the analyzer team wasn't completely pleased with the plugin architecture today.

What gave you that impression?

@matanlurey
Copy link
Contributor Author

@bwilkerson:

... wouldn't take much effort (on the order of days).

Nice!

... the analyzer team wasn't completely pleased with the plugin architecture today.

What gave you that impression?

I was told there were potential memory usage issues (having to load another isolate with entire analyzer source code alone in Dart 2 is very expensive), with some questions around how it would share the same version of the cache. Maybe that concern was IDE-specific though?

@bwilkerson
Copy link
Member

I do want to do some performance testing to know whether there are issues with the current implementation, but I'm very happy with the current architecture of having plugins in a separate isolate and using a JSON-RCP like interface to communicate between server and plugin.

... with some questions around how it would share the same version of the cache.

That's an issue for the angular plugin because it wants (needs?) to add extra data to the cache and we don't currently support that, but I think most plugins can share the same on-disk cache without any trouble.

@matanlurey
Copy link
Contributor Author

Got it! Well that makes me feel a lot better for this use case, and I'd love to contribute to help start up said "Google3 Plugin" if that ends up being the path we want to take for some of these rules. I'll start an internal thread and assume this is preferred stance right now :)

@devoncarew
Copy link
Member

As a drive by comment, we have added some google3 specific functionality to the pkg/analysis_server/bin/server.dart file. Patching the internal server does have drawbacks, but some benefits as well. It would be useful to compare that with the wire protocol plugin technique, in order to best measure the memory and CPU (and maintainability) impact.

@a-siva a-siva added the legacy-area-analyzer Use area-devexp instead. label Aug 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
customer-google3 devexp-linter Issues with the analyzer's support for the linter package legacy-area-analyzer Use area-devexp instead.
Projects
None yet
Development

No branches or pull requests

4 participants