Better error message for missing space before semicolon in requirements #1746
Conversation
PEP 508 requires a space between a URL and the semicolon separating it from the markers to disambiguate it from a url ending with a semicolon. This is easy to get wrong because the space is not required after a plain name of PEP 440 specifier. The new error message explicitly points out the missing space. Fixes #1637.
crates/pep508-rs/src/lib.rs
Outdated
| // Unwrap safety: The `VerbatimUrl` we just parsed has a string source. | ||
| if url.given().unwrap().ends_with(';') && marker.is_none() { | ||
| return Err(Pep508Error { | ||
| message: Pep508ErrorSource::String("Missing space before ';'".to_string()), |
There was a problem hiding this comment.
This tells you what's gone wrong, but not why we need to issue an error here in the first place. I think if I were a user and I got this message, my immediate question would be, "Well, you obviously understood what I meant here... so why not just do that?"
Maybe something like this? Not sure if there's a more concise way of explaining it:
| message: Pep508ErrorSource::String("Missing space before ';'".to_string()), | |
| message: Pep508ErrorSource::String("\ | |
| Missing space before ';' creates an ambiguous requirement | |
| (Unclear whether the ';' is part of the URL or separates the URL from an environment marker)".to_string() | |
| ), |
There was a problem hiding this comment.
The reason itself is somewhat counterintuitive since we allow paths too, which unlike urls can contain spaces which we don't support except for the hack with file urls, and because PEP 508 doesn't require a semicolon after a version number for symmetry.
| @@ -976,12 +982,26 @@ fn parse(cursor: &mut Cursor, working_dir: Option<&Path>) -> Result<Requirement, | |||
| // wsp* | |||
| cursor.eat_whitespace(); | |||
| if let Some((pos, char)) = cursor.next() { | |||
| if let Some(VersionOrUrl::Url(url)) = requirement_kind { | |||
| // Unwrap safety: The `VerbatimUrl` we just parsed has a string source. | |||
| if url.given().unwrap().ends_with(';') && marker.is_none() { | |||
There was a problem hiding this comment.
Can we just make this if let rather than unwrap?
PEP 508 requires a space between a URL and the semicolon separating it from the markers to disambiguate it from a url ending with a semicolon. This is easy to get wrong because the space is not required after a plain name of PEP 440 specifier. The new error message explicitly points out the missing space.
Fixes #1637