dart-lang / tools

This repository is home to tooling related Dart packages.
BSD 3-Clause "New" or "Revised" License
25 stars 20 forks source link

Make `config.yaml` optional #286

Open rrousselGit opened 1 month ago

rrousselGit commented 1 month ago

Hello!

I was working on a tool that needs plugins (custom_lint) and considered using extension_discovery. But in my case, the config.yaml is useless, as plugins will provide separate files inside (a lib/main.dart).
Asking my users to specify a non-empty config.yaml just for the sake of it feels wrong.

Could we make it optional? The presence of that extension/<name> folder appears to be enough to detect plugins.

rrousselGit commented 1 month ago

I'm open to making a PR for this. It appears straightforward.

devoncarew commented 1 month ago

cc @kenzieschmoll, @jonasfj

jonasfj commented 1 month ago

I would suggest not doing this.

The code in package:extension_discovery relies quite a bit on modification timestamps in order to do things fast. Especially, if you have path-dependencies which are mutable, meaning it's not enough to check the timestamp of package_config.json (against the cache file).

I'd suggest that an empty config.yaml isn't the worst thing we could do.


How about finding something to use config.yaml for? :rofl: :see_no_evil: :rofl: (yes, that sounds a little bit stupid, hehe)

But it's kind of likely that in some future you'll find a need for it.

If a package includes more than one lint rule, then maybe it'd be nice if those are declared in a config.yaml. Ideally, custom_lint could avoid loading lints that aren't enabled.

Or maybe, you decide that custom lints should implement a different interface, or they should put the lint implementation in lib/src/custom_lint.dart instead of lib/main.dart, or whatever.

If you decide to make such a change you'd have to make a breaking change. Unless, you add a version number in config.yaml, then custom_lint could be made such that it can load both the new style lints and the old style lints.

Of course, that might not be super relevant to you right now, because breaking changes probably happen relatively frequently due to the package:analyzer dependency.

jonasfj commented 1 month ago

also, checking empty folders into git can be a bit less fun. So you might end up with a different empty file (like a .gitkeep file).

Until recently pub publish also had an issue with empty folders, I think we've fixed that, but it might not be in stable until Dart 3.5

jonasfj commented 1 month ago

Actually, in your case, if config.yaml listed all the rules that a lint-package provides, then it'd possible to make a command that can quickly list all the custom lint rules you can enable.

Loading all the custom lints just to print out what the names of the rules are is probably a bit slow.

rrousselGit commented 1 month ago

Actually, in your case, if config.yaml listed all the rules that a lint-package provides, then it'd possible to make a command that can quickly list all the custom lint rules you can enable.

That's redundant. I can list lint rules and configure them without such a file. I truly have no use for the file. The only thing this file would do for me is make things worse for package authors by requiring extra steps.

I'd suggest that an empty config.yaml isn't the worst thing we could do.

An empty file doesn't work ;) Due to how the package filters "null" configs (and an empty file will be parsed as null), the config file has to be non-empty

also, checking empty folders into git can be a bit less fun. So you might end up with a different empty file (like a .gitkeep file).

Sure but the directory is bound to have something it is. Otherwise what's the point of the plugin? It's just that the config.yaml is useless in our case.

jonasfj commented 1 month ago

I'll outline arguments worth considering as:

I'll admit that most of my arguments sum up to: slightly more options is slightly more complexity -- is that worth it?

Is there much cost to keeping a config.yaml file?

Sure but the directory is bound to have something it is. Otherwise what's the point of the plugin?

You might have all the files implementing the plugin in lib/src/. Isn't that what custom_lint does today?

I personally think that providing meta-data along with an extension is a very natural thing. And that loading meta-data by compiling a program containing all the extensions and then running it as a subprocess is pretty slow. extension_discovery goes to great lengths to cache the meta-data and ensuring its consistent after a new pub get. I guess the compiler also does some caching, it's just not quite as fast (yet, hopefully one day).


I can be convinced if others feel strongly. I just don't personally think this is necessarily worth doing.

Is it really that bad?

On Wed, Jul 17, 2024, 21:47 Remi Rousselet @.***> wrote:

Actually, in your case, if config.yaml listed all the rules that a lint-package provides, then it'd possible to make a command that can quickly list all the custom lint rules you can enable.

That's redundant. I can list lint rules and configure them without such a file. I truly have no use for the file. The only thing this file would do for me is make things worse for package authors by requiring extra steps.

I'd suggest that an empty config.yaml isn't the worst thing we could do.

An empty file doesn't work ;) Due to how the package filters "null" configs (and an empty file will be parsed as null), the config file has to be non-empty

also, checking empty folders into git can be a bit less fun. So you might end up with a different empty file (like a .gitkeep file).

Sure but the directory is bound to have something it is. Otherwise what's the point of the plugin? It's just that the config.yaml is useless in our case.

— Reply to this email directly, view it on GitHub https://github.com/dart-lang/tools/issues/286#issuecomment-2234114769, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABERZFWKAAGBNQA5NGEIVDZM3C3LAVCNFSM6AAAAABK7PA3XGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMZUGEYTINZWHE . You are receiving this because you were mentioned.Message ID: @.***>