-
Notifications
You must be signed in to change notification settings - Fork 64
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(api): add lfs metadata #8
Conversation
Can we add a test for it ? (Also so I can have a look at what models return that could be interesting in other places to get file sizes without Range:0-0. |
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.
Quite nice.
I propose a different (simpler imho) internals.
Otherwise looks good. Not sure how cargo fmt
didn't pick up those before.
src/api/sync.rs
Outdated
|
||
/// serde_json error | ||
#[error("Serde json error: {0}")] | ||
SerdeJson(#[from] serde_json::Error), |
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.
How were we getting away without it before, is it the map(Box?) ?
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.
Response::into_json
returns an io::Result<T>
while serde_json::from_value
returns a Result<T, serde_json::Error>
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.
And reqwest::Response::json
returns a Result<T, reqwest::Error>
in favor of returning raw request builder
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
Full lfs object looks like this :
Needed for the repo scanner