Conversation
Codecov Report
@@ Coverage Diff @@
## master #55 +/- ##
=========================================
+ Coverage 0.00% 0.09% +0.09%
=========================================
Files 50 50
Lines 1988 2028 +40
=========================================
+ Hits 0 2 +2
- Misses 1988 2026 +38
Continue to review full report at Codecov.
|
Hirevo
left a comment
Hirevo
left a comment
There was a problem hiding this comment.
This is a very neat abstraction, not having to manually deal with the filesystem and the JSON (de)serialization, just to make a quick change (like yank a crate) is a great step forward.
I'd say using a map is indeed the better choice here than a Vec because accessing records based on the version is quite frequent in our case (more than sequentially iterating them).
According to the documentation page about when to use BTreeMap, it seems the better choice, since we tend to access the latest version the most, currently.
So, yeah, I would agree about using a BTreeMap here.
| .collect::<Result<Vec<String>, json::Error>>() | ||
| .unwrap(); // we unwrap because any error here is a logic bug in the implementation | ||
|
|
||
| let text = lines.join("\n"); |
There was a problem hiding this comment.
I have a doubt about whether we need to also add a new line at the very end of the file.
In the previous version, there was an additional writeln!(file); to add this final newline.
You're right that not having tests makes this hard to validate.
End-to-end tests are coming very soon (I think I have a good model on how to do them and implementation work has begun), but if it doesn't land when this PR is ready to be merged, we'll have to manually test that.
(That review comment should remind me of that.)
| File::create(path) | ||
| } | ||
|
|
||
| pub fn match_record(&self, name: &str, req: VersionReq) -> Result<CrateVersion, Error> { |
There was a problem hiding this comment.
This could also be implemented with File:
self.file(name)
.records()
.iter()
.filter(|(vers, _)| req.matches(vers))
.max_by(|(v1, _), (v2, _)| v1.cmp(v2))
.map(|(_, record)| record.clone())
.ok_or_else(|| AlexError::CrateNotFound {
name: String::from(name),
})
There was a problem hiding this comment.
i'll take a look at this. my inclination would be to push this down to the File. So the tree would be responsible for checking the name exists, and the file for doing the version matching.
also it seems like a mistake to clone in the method. better to return the reference, and let the caller clone it (if they need to)
There was a problem hiding this comment.
quick question- what are you using the match record method for?
I had considered this 'dependency resolution' task to be Cargo's job
2 participants
@Hirevo
this is a first draft of what it might look like to refactor the tree into a 'File' object (for managing specific index files)
this is where it gets a bit murky, as without any tests, the intended semantics are slightly opaque.
also there's a few design decisions in there i'd love to chat about. using a
BTreeMapinstead of a vector for storing crate records, for example.