-
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
Support custom lint rules #697
Comments
cc @bwilkerson re: plugin support. |
+1 👍 💯 Another example from me, |
I think this would be an awesome feature to have! In some way, the customised linter can act like a second compiler which helps to detect more errors/code smell at compile time, leading to fewer runtime exceptions. |
@pq Do you have any vision how to do plugins support? Why linter is a part of sdk now? |
Sorry for the slow reply @stepancar! The original intention of the linter was for it to function as a plugin and be very loosely coupled w/ the SDK. For a variety of reasons, largely performance-related, we had to step back from that. (@bwilkerson can provide some more context.) Anyway, I agree it would still be great to support some kind of model for pluggable analyses even if, for performance reasons, we need to keep the core linter embedded the way it is today. Out of curiosity, what kinds of use cases are you envisioning? |
We wrote an internal linter using the unsupported analyzer APIs that does the following right now:
We've actually had some trouble porting this tool to Dart 2 because the analyzer has changed so much. At the time we wrote it, we met with @kevmoo and he indicated that a pluggable analyzer was coming. I know that became a thing for Flutter, but it isn't considered generally available and the documentation is very sparse, so it still doesn't help us (yet?). I don't think we're picky about how we satisfy these use-cases, we just need to be able to satisfy them in a reasonably stable manner. |
Thanks for the added context @georgelesica-wf! When does your tool get run? (And how regularly?) Are you building on https://github.com/Workiva/dart_dev ? FYI @bwilkerson |
It's run by a global command: We can run that locally at will, but we also add it to our dart_dev task runner command and run it in CI. The types of things we check for are things that we'd love to have available in real-time in our IDE while developing, but we're making do for now, hoping we could get that sort of functionality for free from analyzer plugin support :) |
To expand on your last question @pq it doesn't build on top of dart_dev directly. It actually does use our semver auditing tool as a library to get the compiled elements. Unfortunately, that's the part that has proven difficult to update to Dart 2. |
If there are specific questions about the analyzer APIs and how to update them, I'm more than happy to answer them for you. (The API is continuing to evolve to better represent the Dart 2.0 semantics and to allow for performance improvements in the implementation, and I might also be able to help you future-proof your code.) I'm happy to use any forum you want, but I'll see github issues and e-mail without prompting; for other forums you might need to let me know that there's something to respond to.
Yes, we do have a plugin story for the analyzer now. It isn't Flutter specific, and Flutter isn't currently using it. It is currently being used to support Angular. It's currently only being used by the analysis server, but we have (unscheduled) plans to add support for it to the command-line analyzer as well at some point. We haven't made it generally available (encourages people to use it) because
I don't know when we might be able to address those concerns. |
Any updates? Custom rules could improve code consistency on a particular project. I think it's a must-have feature for any linter. Also, it will allow to make a platform or framework specific rules. Flutter, Aqueduct or DartAngular rules could be very helpful. |
@cah4a: no significant updates. I'd still love to see this happen (for many of the reasons you describe and a few more). Our current energy has been focussed on a scramble to keep up with (and support) evolving Dart language features but continued conversation here (and upvotes) will help motivate prioritization down the road, so thanks for that! Regarding convention checks, is it possible that some of your desired ones would be generally useful? Are they worth considering implementing in the linter proper? As for Flutter rules, could you enumerate ones you have in mind? (Folks are actively thinking about those so your ideas will be especially welcome -- #142 is an old tracking issue too fwiw.) Finally, short of tight integration of custom lints into the IDE could you see value in a way to run custom lints from a separate (command-line) tool? A flow like @georgelesica-wf describes? |
Look at the scramble I guess overloaded functions is not in the near sight
Atenciosamente,
Jonathan Rezende
… Em 6 de abr de 2019, à(s) 10:11, Phil Quitslund ***@***.***> escreveu:
@cah4a <https://github.com/cah4a>: no significant updates. I'd still love to see this happen (for many of the reasons you describe and a few more). Our current energy has been focussed on a scramble to keep up with (and support) evolving Dart language features <https://github.com/dart-lang/language/projects/1> but continued conversation here (and upvotes) will help motivate prioritization down the road, so thanks for that!
Regarding convention checks, is it possible that some of your desired ones would be generally useful? Are they worth considering implementing in the linter proper?
As for Flutter rules, could you enumerate ones you have in mind? (Folks are actively thinking about those so your ideas will be especially welcome -- #142 <#142> is an old tracking issue too fwiw.)
Finally, short of tight integration of custom lints into the IDE could you see value in a way to run custom lints from a separate (command-line) tool? A flow like @georgelesica-wf <https://github.com/georgelesica-wf> describes?
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub <#697 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AF6Y3s3HZCjQ-c0N9ci0_yY7CBAzju_eks5veJ0fgaJpZM4N0rfT>.
|
@pq At the moment, I want to implement some "Separation Of Concerns" checks. Eg:
So it's not about what code does, but where it lives. Don't think it could be generally useful. Of course, I want to have custom lint rules because of tight IDE integration. So you could see those issues while you coding. Otherwise, you try to satisfy CI after a commit is ready. That could be annoying. As for flutter rules, I think the next could be useful:
|
See also flutter/flutter#1220 |
Any progress on this? This seems like something that many users can benefit from. |
Not much unfortunately. Maybe once we're through extension methods and NNBD? 🤞 (That's the team-wide priority for the immediate term.) |
Is there any progress on this, we need custom dart lint rules too, it is not universal for everyone, but it is a very meaningful constraint for our team's project |
No, there hasn't been any progress on this. While extensions have shipped, NNBD is still very much our primary focus right now. But more generally, I'm honestly not sure what support for custom lints would look like. Every time I think about what would be required in order to support custom lints I come to the same conclusion: it would look exactly like what we already support with analyzer plugins. Unless we could do something that would make it easier for users (and that can't be applied to the analyzer plugin support to also make it easier) I don't think we're likely to do anything different. I generally discourage people from using our plugin support because we have not finished evaluating the impact that plugins have on the performance of the analysis server, nor are plugins used by the command-line analyzer. The alternative is to write your own linter-like support using the analyzer package. It won't give you the IDE integration that analyzer plugins were designed to provide, but you could, at least, run them from the command-line and as part of your CI system. |
Thanks Brian!
If you do want to explore this route, you might find this package useful: https://github.com/pq/surveyor Adding custom analyses is what this is all about and it would be easy to create a script to run in a CI. Feel free to reach out if you have any questions. |
Yeah I think it would be great to write custom rules as it is possible Android Lint. I had rules like the following in my mind:
Essentially very project setup specific rules that should help new team members to get onboarded and follow team internal rules. |
Are there any relevant updates in this feature? |
fwiw, using
although this is unfortunately not correct, as plugins do not work with the CLI right now: |
~4 years but still no result ... that's quite sad 😢 |
@hpoul your package was helpful -- I was able to almost get our own analyzer plugin stood up, but the IDE highlighting would show up and then randomly disappear. This wasn't the case when messing around with
On the CLI front, we were able to use most of Edit: You can see here how after an issue shows up successfully, removing and replacing the issue does not cause it to be highlighted again... Screen.Recording.2021-10-27.at.4.13.18.PM.mov |
Are you able to make a small repro you can share that triggers this? (it would make sense to file it on an issue at https://github.com/dart-lang/sdk) |
Yeah, I was thinking the same. I'll dig into doing that tomorrow. Shouldn't be too hard at all! |
@DanTup I was able to put together a small reproduction project using Screen.Recording.2021-10-28.at.10.34.07.AM.movHere's the diagnostics One additional thing that I noticed, is that when the analysis server creates a directory for the analyzer_plugin, it copies over it's
Will not be able to resolve things packages that exist in the host package because that package is not copied over to the temp directory as well. Am I off base/misunderstanding something here? In the case of
If I'm correct, does this mean that you cannot use relative paths when referencing the host package from the bootstrap backage? PS I didn't want to create an issue quite yet in case you weren't able to reproduce, but let me know if you are and I'd be happy to open one with this information. |
@btrautmann I don't seem to be able to get the plugin working at all. If I enable the analyzer instrumentation log I see this:
I believe It's probably worth moving this to an SDK issue to avoid adding lots of noise here. If it turns out to not be an issue, it can always be closed (and if it's something you've done, having a recorded issue with the solution may be useful for others searching there too).
I'm not certain if this is intended or just hasn't come up before. @bwilkerson may be better placed to answer that. |
Ah, interesting -- I was running into this last night in my other project trying to get things working and after I ran a |
Update: I do believe, as mentioned in dart-lang/sdk#47567 (comment), that flakiness was caused by using a relative path within the bootstrap package's pubspec. As far as this issue is concerned, our team was able to write our own analyzer plugin usable via CI and the IDE based on dart-code-metrics. Obviously this is a heavy-handed approach for every team using dart/Flutter to do. Our team is interested in possibly working on a way to make this easier for the community (dart code metrics has said they don't have interest in supporting custom lint rules), but we're not committed to it yet. |
Dart Code Metrics here. Just to make it clear: out point is that a proper direction would be to have an easier way to create plugins rather than creating some additional package with api that depends on the plugins api. This approach is very fragile. Also, consider that if someone adds a linter rule to linter package or DCM, for instance, there will be no need for others to implement the same rule from scratch. Looks like a huge benefit for the community to me.
And to point it out clearly: DCM is a set of custom lint rules (among other features) and I'm really curious, what stops you from suggesting a rule and contributing instead of creating a whole new package? |
For me, the point of being able to create a package would be to develop project-specific rules. For example naming conventions, where the linter would aways tell me or someone in my team not to create |
Event though I got the idea, looks like your case can be covered with a generic rule like https://dartcodemetrics.dev/docs/rules/common/ban-name. Or am I missing something? |
That's basically what I was looking for, thank you!
That's basically what I was looking for, thank you! |
Yeah, along the same lines as what @FMorschel is saying above, we have project-specific rules based on internal APIs that we'd want to enforce, e.g use I definitely apologize if I made it sound like DCM was "unwilling" or "rudely against" a more abstract solution. Definitely not my intention. Your project is awesome and helped us HUGELY with our own implementation. I totally understand a desire not to build out a less-than-ideal solution like the one proposed above. I do think, though, that the "easier analyzer plugin" approach will probably be the only viable one for the foreseeable future. Out of curiosity, assuming you shipped as part of that solution a set of stable APIs that consumers could utilize, what are the exact concerns around fragility? Asking because it could inform (or deter us from) our approach. |
Oh, no worries, I just tried to give an additional context for others.
Thank you!
Unfortunately, current plugins API is not that mature and sometimes has unexpected problems (mostly performance, but in early days we also copied some hacky code from Dart Angular plugin just to make ours work). I expect this kind of problems will resolve with time, especially if there will be more plugins (big thanks to the Dart team, they're really investing in making plugins great), but here are few examples of issue I'm talking about: dart-lang/sdk#47851, Dart-Code/Dart-Code#3679, dart-lang/sdk#48682. If anything like that breaks, you might get your users become really unsatisfied with the stability of your package. On the other hand there are some limitation from the API, like: no way to add custom I think that's why investing in capabilities, stability, documentation / examples, and maybe some additional linting API for plugins might be a better approach, IMO. If nothing of that stops you, take a look at this repo https://github.com/hisaichi5518/dart-linting. This might give you some ideas too. |
That is, unfortunately, true. It wouldn't be too hard, though, to add a protocol to support that case.
|
Invertase published a package to make your custom lint rules: https://invertase.io/blog/announcing-dart-custom-lint 🎉 cc: @rrousselGit |
If I'm understanding how things work under the hood (I haven't looked at the source, but only the announcement blog post, so I could be wrong), this is very similar to what our team was thinking of doing, and probably has the risks that @incendial was warning about regarding building against an unstable API. That said, I think it's a great step in the right direction for the community at large and is the only reasonable next step at this time. Our team ended up basically doing a pared own fork of dart code metrics, added the ability to baseline, and added the ability to parse/lint YAML files, which I don't think would be possible with Invertase's solution (at least out of the box). So for folks like us that want a more advanced feature set, it probably doesn't make sense to migrate, but for 99% of the community I think this is awesome! |
Thanks! ** Acknowledge invertase |
@brianquinlan this sounds pretty lit 🔥 , we have the same in our roadmap |
I don't see why you couldn't. custom_lint does it. When plugins are starting or fail to start, it underlines the plugin name in the project's pubspec.yaml |
Totally, that's why I suffixed that statement with "(at least out of the box)" 😊 |
Would love to see some progress on this. It'd be really great if custom lint rules created with custom_lint would be run with |
I'd like to be able to specify custom lint rules outside of this package. This is mostly to help enforce consistency in consumption patterns of some of our private libraries that don't apply well to the larger Dart community, but would be very beneficial to our developers.
To give 1 specific example, we've implemented a
Disposable
interface / mixin to assist with cleaning up streams and other data structures that won't necessarily be garbage collected without some manual intervention (https://github.com/Workiva/w_common). I'd like to be able to perform some linting similar to the existingcancel_subscriptions
andclose_sinks
rules to verify that anyDisposable
object instantiated is actually disposed.The text was updated successfully, but these errors were encountered: