Skip to content

Deprecate --packages flag #484

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
liamappelbe opened this issue Mar 8, 2022 · 9 comments · Fixed by dart-archive/coverage#370
Closed

Deprecate --packages flag #484

liamappelbe opened this issue Mar 8, 2022 · 9 comments · Fixed by dart-archive/coverage#370
Assignees

Comments

@liamappelbe
Copy link
Contributor

This flag takes either the .packages or package_config.json file directly. This is a bit inconvenient, especially since .packages is being discontinued, and package_config.json is inside the .dart_tool directory, which users shouldn't have to know the details of.

Instead we should have a --package flag that accepts the path to the root of the package, and uses package_config.findPackageConfig to locate the package config.

@mit-mit
Copy link
Member

mit-mit commented Mar 9, 2022

I think we can go ahead and not only deprecate, but fully discontinue/remove this flag. That is what we're doing elsewhere (and what breaking change dart-lang/sdk#48272 announced).

@liamappelbe
Copy link
Contributor Author

Ok, I'll delete the --packages flag. But I think I'll still keep support for passing the .packages or package_config.json file into the resolver API, until we do the next major version, since removing support for that is a breaking change.

@mit-mit
Copy link
Member

mit-mit commented Mar 14, 2022

@liamappelbe we're doing a breaking change. It's already been announced and approved. So you can go ahead and remove.

@liamappelbe
Copy link
Contributor Author

Are you saying this breaking change also covers breaking API changes in package:coverage?

@natebosch
Copy link
Member

Dropping support for passing in package_config.json paths entirely (or package paths if we go that route) would be more breaking. The SDK breaking change does not remove without replacement - the replacement already exists.

@liamappelbe
Copy link
Contributor Author

@mit-mit I think there's 2 questions here:

  1. Should we delete the --packages flag, or just deprecate it until the next major version of package:coverage?
  2. Should we delete Resolver.packagesPath, or just deprecate it?

I have a PR out that deletes --packages, but just deprecates Resolver.packagesPath, but I'm getting push back on deleting --packages. I don't think the breaking change covers changes to package:coverage, so we should probably just deprecate both of those things, and not delete anything yet.

@mit-mit
Copy link
Member

mit-mit commented Mar 21, 2022

The breaking change is meant to cover all things .packages. We've had the replacement for it (package_config.yaml) for years so as Nate mentioned you can just pass that instead. I don't see why we wouldn't just go ahead and remove any code path that is specific to .packages assuming the corresponding code path based on package_config.yaml is already functional.

@natebosch
Copy link
Member

I don't see why we wouldn't just go ahead and remove any code path that is specific to .packages assuming the corresponding code path based on package_config.yaml is already functional.

SGTM.

  • Should we delete the --packages flag, or just deprecate it until the next major version of package:coverage?
  • Should we delete Resolver.packagesPath, or just deprecate it?

I think both of these are included in the code path for package_config.yaml so removing it shouldn't be necessary. Adding --package-root as another option would be fine, but I don't think we need to delete --packages or existing arguments which will continue to work with the modern file format.

Maybe all we need to do is delete the small chunk of fallback parsing.

https://github.com/dart-lang/coverage/blob/aab92ee30434f131719aae0314cf5413b4997f2d/lib/src/resolver.dart#L98-L111

@liamappelbe
Copy link
Contributor Author

Ok, so I'll delete the .packages fallback parsing in the resolver, and deprecate (but keep for now) the packagesPath argument in the resolver and the--packages command line flag. Then at the next major version of package:coverage we can delete the deprecated stuff.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants