Skip to content
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

Updated tag detection for packages without a primary library. #592

Merged
merged 6 commits into from
Dec 23, 2019

Conversation

isoos
Copy link
Collaborator

@isoos isoos commented Dec 16, 2019

  • Fixes linter: missing sdk.version... a little help? #591.
  • Top-level libraries are the files in lib/*.dart. However, if there is a primary library in the package (/lib/[package_name].dart), it will be used as a single value in top-level libraries.
  • Tags are added when all of the top-level libraries agree on supporting the given SDK or platform.
  • Binary-only packages are handled via a temporary heuristic.

@pq
Copy link
Member

pq commented Dec 16, 2019

Thank you @isoos!

Copy link
Contributor

@sigurdm sigurdm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for looking into this!

LGTM for the short-term, but ideally we should recognise packages that are mainly executable and have another tagging scheme for them, or at least use their declared executables from the pubspec as root libraries.

lib/src/tag_detection.dart Outdated Show resolved Hide resolved
@isoos
Copy link
Collaborator Author

isoos commented Dec 17, 2019

ideally we should recognise packages that are mainly executable and have another tagging scheme for them, or at least use their declared executables from the pubspec as root libraries.

I'm not entirely sure that we should treat them that much different on this stage of the analysis. E.g. in case of package:linter, while it may be a "binary-only" package, I can still import its internals (lib/src/.../whatever.dart), use them, and they may just work fine.

Of course we should also tag "binary-only" status, which would change the instructions on the "Install tab"...

@sigurdm
Copy link
Contributor

sigurdm commented Dec 17, 2019

I can still import its internals (lib/src/.../whatever.dart), use them, and they may just work fine.

You can, but the convention is to not import internals, and I don't think we should provide an analysis that suggests what works in case you break the convention.

@isoos
Copy link
Collaborator Author

isoos commented Dec 17, 2019

That's a good point. Should I remove the lib/src part from this one, or are we doing that later?

@sigurdm
Copy link
Contributor

sigurdm commented Dec 17, 2019

Could you look up the executables, and use those in case there are nothing in lib/ ?

@isoos
Copy link
Collaborator Author

isoos commented Dec 17, 2019

I've updated the PR, checking the bin/ directory for files, but only with a (probably temporary) heuristic:

  • not checking the pubspec.executables, because it is optionals, and many binary packages, including package:linter and my own codegen library ignored it (it is useful for having an alias, but not a requirement for pub run ...)
  • it returns sdk:dart and runtime:native-* as tags, without really checking the referenced libraries

@isoos isoos requested a review from sigurdm December 17, 2019 16:44
@isoos isoos merged commit 44a272d into dart-lang:master Dec 23, 2019
@isoos isoos deleted the primary branch December 23, 2019 10:54
@pq
Copy link
Member

pq commented Dec 23, 2019

Fantastic! Any chance you could push out a release with this fix?

Much appreciated!

@isoos
Copy link
Collaborator Author

isoos commented Dec 23, 2019

@pq: Unfortunately there are no production deployments around the holidays, I'd expect this to get into prod in early January.

@pq
Copy link
Member

pq commented Dec 23, 2019

No worries. That makes perfect sense.

Hope you enjoy some time off! ❄️

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

Successfully merging this pull request may close these issues.

linter: missing sdk.version... a little help?
3 participants