-
Notifications
You must be signed in to change notification settings - Fork 15
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
Introduce snapshots to distribute advisories (#170) #179
base: main
Are you sure you want to change the base?
Conversation
@frasertweedale I think the failure is expected as the file is oob (not tracked in Git history) |
2feae57
to
24bd053
Compare
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 should consider being more careful in pulling in new dependencies, either depends on a whole load of things, admittedly the dependencies are quite low in the dependency tree, usually, but I don't think it's justified here.
A few more informative comments/ docstrings wouldn't hurt either, to increase readability.
I don't think switching the toml
parsing library is a good idea at all. e.g. because it doesn't even support toml 1.0.0 or it actually does wrong parses in our case (see my review).
I also think treewide formatting changes in files that get heavily changed should be done in a separate PR
.
resultE <- try $ get $ repoUrl </> "commits" </> branch </> "advisories.atom" | ||
resultE <- try $ get $ mkUrl [repoUrl, "commits", branch, "advisories.atom"] |
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.
what is the advantage of this as compared to https://hackage.haskell.org/package/filepath-1.5.2.0/docs/System-FilePath.html#v:-60--47--62-
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.
filepath
is OS-dependant, here we are dealing with URL, I did not want to pull a new library just for that.
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.
ah, this is an URL
, sure; I think with these error prone things, it would actually be useful to pull a new library, in contrast to extra
or either
which provide trivial combinators
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 did not find a proper library, do you have any suggestions?
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.
@hasufell: I believe you had to deal with URLs/URIs in the recent past. What would you recommend?
722d7fe
to
534c3ce
Compare
534c3ce
to
5f0cef1
Compare
@frasertweedale FYI, I have revert back to |
hsec-tools