Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
yarn: Automated Yarn version detector #743
yarn: Automated Yarn version detector #743
Changes from all commits
ceb2583
e5f3038
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need a unit test for this function, as the coverage is very low on this module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recently, I got a comment from Erik, that all custom cachi2 exceptions should be inside
cachi2.core.errors.py
. But that's just a nitpick.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correction - I meant any publicly exposed top-level exceptions like
PathOutsideRoot
,PackageRejected
, etc. ...custom exceptions which are module-scoped are fine to keep in the given module where it makes the most sense for them. That said, these are module-scoped exceptions and so we should start their name with an underscore. Additionally, these should be re-raised in thefetch-yarn-...
meta entrypoint as aPackageRejected
exception IMO as that's the public-facing one.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are not module-scoped, but they are also not cachi2-scoped: they are raised here to send a message to meta-PM. They do not need to be re-wrapped because as we have agreed earlier everything that is not v1 is v2+ (and exceptions from v2+ propagate freely). Furthermore, any other exception will propagate to the user, so if something happens to conform to v1 and not trigger any of these, and also is a broken v1 then a user would know.