-
Notifications
You must be signed in to change notification settings - Fork 6
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
feat: support analysis of URLs #246
Conversation
wdl/src/bin/wdl.rs
Outdated
@@ -103,7 +105,15 @@ async fn analyze<T: AsRef<dyn Rule>>( | |||
}, | |||
); | |||
|
|||
analyzer.add_documents(vec![path]).await?; | |||
// Convert the file to a either a local URL or a remote URL | |||
let file = if file.starts_with("http://") || file.starts_with("https://") { |
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.
Is this sufficient? What about other protocols such as FTP? Or URLs that omit the leading protocol?
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.
Would it be better to just always call Url::parse()
, and if that fails attempt PathBuf::from()
(and if that fails, bail!()
)?
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.
Just typed up the following and it seems to work:
let file = if let Ok(url) = Url::parse(&file) {
url
} else if let Some(url) = path_to_uri(&PathBuf::from(file.clone())) {
url
} else {
bail!("failed to convert `{file}` to a URI", file = file)
};
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.
That's what I was wondering. I'm curious to hear other thoughts on this, but it makes sense to me.
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.
I think it makes sense to attempt to parse it as a URI and fallback to treating it as a path (also, path_to_uri
should probably be changed to take impl AsRef<Path>
instead of &Path
).
Regarding the supported schemes for the URIs, I think only http
, https
, and file
should be supported as it's the only schemes required by the standard for import statements (with file
currently deprecated and http
should definitely be deprecated, but isn't currently).
Co-authored-by: Andrew Thrasher <[email protected]>
Co-Authored-By: Andrew Thrasher <[email protected]>
…bs/wdl into feat/analyze-urls
Co-Authored-By: Peter Huene <[email protected]>
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.
Looks good, just the nit comment.
Co-Authored-By: Peter Huene <[email protected]>
Adds remote file support to
wdl-analysis
and thewdl
binary.With this change, we have the option to stop cloning repos in Gauntlet.
Before submitting this PR, please make sure:
changes (when appropriate).
CHANGELOG.md
(see"keep a changelog" for more information).