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

fix: jsr specifier does not support tags #390

Merged
merged 1 commit into from
Feb 20, 2024
Merged

Conversation

irbull
Copy link
Contributor

@irbull irbull commented Feb 17, 2024

When a jsr specifier is used, an error is returned if the version segment contains a tag.

Fixes: denoland/deno#22420

@CLAassistant
Copy link

CLAassistant commented Feb 17, 2024

CLA assistant check
All committers have signed the CLA.

@irbull
Copy link
Contributor Author

irbull commented Feb 17, 2024

It would be good to create an integration test that shows the error attached to the graph. Any pointers?

src/graph.rs Outdated
@@ -2770,6 +2773,14 @@ struct Builder<'a, 'graph> {
diagnostics: Vec<BuildDiagnostic>,
}

#[derive(Error, Debug, Clone)]
pub enum JsrPackageFormatError {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if this is the best way to propagate errors. Suggestions welcome.

Copy link
Member

Choose a reason for hiding this comment

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

This is good thanks! Minor nitpick: would you be able to move it down to above validate_jsr_specifier so it's not between Builder and its implementation though?

src/graph.rs Outdated Show resolved Hide resolved
@dsherret
Copy link
Member

dsherret commented Feb 17, 2024

Probably easiest would be to make a copy of one of these files:

https://github.com/denoland/deno_graph/blob/main/tests/specs/graph/JsrSpecifiers_VersionNotExist.txt

Then modify and run with UPDATE=1 cargo test then inspect the changes it makes.

src/graph.rs Outdated
.map_err(|err| JsrPackageFormatError::JsrPackageParseError(err))?;
match package_ref.req().version_req.inner() {
RangeSetOrTag::Tag(_) => {
Err(JsrPackageFormatError::JsrPackageVersionTagNotSupportedError)
Copy link
Member

Choose a reason for hiding this comment

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

To reduce verbosity we could strip JsrPackage and Error from the variant's name (ex. JsrPackageFormatError::VersionTagNotSupported).

When a jsr specifier is used, an error is returned if the version
segment contains a tag.

Fixes: denoland/deno#22420
Copy link
Member

@dsherret dsherret left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @irbull!

@dsherret dsherret merged commit 319dd59 into denoland:main Feb 20, 2024
4 checks passed
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.

Deno panic when importing jsr package with @next label
3 participants