-
Notifications
You must be signed in to change notification settings - Fork 2
Conversation
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.
Looking awesome! Just a couple of little things you can do to avoid some copying, and make some other copying you can't avoid a bit more obvious. 👍
Great work! 👏
reference, | ||
}) | ||
.map(|x| { | ||
x.0.map(Binary::from) |
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.
Don't mind if we fix in another PR — but would be inclined to use a struct here instead of a tuple, x.0
is kind of gross because it's hard to see from the code what it is that is being operated on.
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'm usually anti-tuple so happy to refactor now that you've called me out on it 😛
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.
Wait, got mixed up, was thinking of the tuple I used here. Though in this case it's necessary for building the map.
Line 48 in de7bf87
Repository::open(path).ok().and_then(|repo| Some((name, repo))) |
Given the sprint ends tomorrow I'll pick this up in a future PR?
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.
LGTM! 👏
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 noticed this failed some clippy lints:
Fixed |
First it populates the list based on the repos found in the repo_root directory specified as parameter. Later, when calling the endpoint, the name of the repo is checked against that repo list and it will only accept requests with a valid repository as parameter. This commit should fix the vulnerability mentioned in #12. Still need to add some tests on the code and some other stuff mentioned as TODOs. Signed-off-by: Albert Pastrana <albert.pastrana@intenthq.com>
Makes `reference` parameter when requesting files optional, defaults to master
Replace mutable loop with functional code Add handling for more failure cases Silently discard repos that failed to load
Make error message more specific Make dependency version more specific Make cloning of data explicit
5ba23a8
to
0d72872
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.
LGTM! 👍
Restricts git repos to a pre-defined selection, as per #12. Worth noting there's no tests for
load_repos
. Do we want to test this method and if so how?This PR also: