fix(clobber registry): directory and file clobbering#1497
fix(clobber registry): directory and file clobbering#1497baszalmstra merged 6 commits intoconda:mainfrom
Conversation
|
This is shaping up very nicely! Awesome work so far! Much clearer! |
remimimimimi
force-pushed
the
fix-clobbering
branch
6 times, most recently
from
bf1c6e3 to
5e95fb5
Compare
remimimimimi
force-pushed
the
fix-clobbering
branch
from
5e95fb5 to
5a93a81
Compare
|
I changed implementation of reprioritize method so now we have performance on par with previous implementation. I'm also much more confident that this function is correct. 
|
This one should handle file vs directory conflicts as well as directory merging correctly most of the time. There are still several places where I think code can fail. And I think follow up should be, more rigorous, property-based testing for `path_trie` crate. Legends say that 2nd generation was lost somewhere in the sea of rebases.
remimimimimi
force-pushed
the
fix-clobbering
branch
from
5a93a81 to
e7320da
Compare
baszalmstra
left a comment
baszalmstra
left a comment
There was a problem hiding this comment.
I really like the structure and your approach! I left a few comments.
| assert_yaml_snapshot!(prefix_record_clobber_2.files); | ||
| assert_yaml_snapshot!(prefix_record_clobber_2.paths_data); |
There was a problem hiding this comment.
Having multiple snapshots in a single test can be annoying because you run the test, get a snapshot change, update the snapshot and assume the test succeeds now. However, another snapshot might still fail. If possible I prefer to combine snapshots in a single snapshot to avoid this confusion.
There was a problem hiding this comment.
See https://insta.rs/docs/cli/#test. I don't think that it would be easy or actually useful to combine snapshots.
If you want to use nextest runner, then you can call cargo insta test --test-runner nextest --review.
There was a problem hiding this comment.
I dont particularly agree that people should just change their workflow, but this is fine for me.
remimimimimi
force-pushed
the
fix-clobbering
branch
from
2413589 to
3cce4df
Compare
We change names to more intuitive as well as fix some PR comments along the way.
remimimimimi
force-pushed
the
fix-clobbering
branch
from
3cce4df to
00f1325
Compare
Since `new` is called on existing prefix records this means that they were reprioritized based on the topological sort, so we have to do the same this during the creation of `ClobberRegistry` in order to have synced on-memory and on-disk representations.
3 participants
Description
Introduce new path conflict resolver that would allow to gracefully resolve path conflicts between file/directory in addition of previously handled file/file conflicts. See tests in
package_path_resolver.rsin order to understand how it will resolve different types of conflict.Current state is draft since we don't do anything on-disk yet.