Conversation
|
This is awesome! We'll review in detail. Thank you @jleben ! |
paraseba
left a comment
paraseba
left a comment
There was a problem hiding this comment.
This looks great! Made a few small comments
icechunk/src/cli/interface.rs
Outdated
| async fn resolve_ref_by_name( | ||
| repository: &Repository, | ||
| reference: &String, | ||
| ) -> Result<SnapshotId> { |
There was a problem hiding this comment.
This is useful logic, and in fact I think we have something similar somewhere. But for now I'd prefer to keep the CLI as logic-free as possible. I'd recommend we only take a snapshot_id as reference, which is what the python API supports today. Then we can add this logic to the main rust code and extend support in the python library and CLI
There was a problem hiding this comment.
I've also renamed the history command to ancestry in the spirit of conforming to the Python API.
| } | ||
|
|
||
| fn show_snapshot(mut writer: impl std::io::Write, snapshot: SnapshotInfo, with_meta: bool) -> Result<()> { | ||
| writeln!(writer, "Snapshot: {}", snapshot.id)?; |
There was a problem hiding this comment.
we have an inspect.rs module. Can we use that instead?
There was a problem hiding this comment.
@paraseba Could you explain what the benefit of using inspect.rs would be? Note that this function prints individual snapshot info in the context of my ancestry command. It (intentionally) does not print all the information about a snapshot. If I was to use inspect_snapshot from inspect.rs for example, it would do more work per snapshot than here. What exactly did you have in mind?
There was a problem hiding this comment.
@paraseba I also wonder what exactly is the intention behind NodeSnapshotInspect - it seems like it contains mostly the same info as NodeSnapshot, but pruned down, simplified somewhat. The level of such a simplification is always going to be specific to some prupose, so I doubt the value of it for multiple purposes. Currently, it seems it's only used in Python's inspect_snapshot. However, a human-friendly display in the CLI may have other needs than the python API. Why try to squeeze a common abstraction between both end-uses and NodeSnapshot?
| let repository = open_repository(&args.repo, config).await?; | ||
| let snapshot = resolve_ref_by_name(&repository, &args.reference).await?; | ||
| let ancestry = repository.ancestry(&VersionInfo::SnapshotId(snapshot)).await?; | ||
| let snapshots: Vec<_> = ancestry.take(args.n).collect().await; |
There was a problem hiding this comment.
I don't think you need to collect here
| } | ||
| } | ||
|
|
||
| async fn inspect( |
There was a problem hiding this comment.
I'd love if we instead reused and extended the logic in inspect.rs
| } | ||
|
|
||
| async fn diff( | ||
| args: &DiffArgs, |
There was a problem hiding this comment.
It would be great if we could DRY things up, some of this logic is present in the python bindings module too: repository.rs, see PyDiff. But maybe this is OK for an initial version
2 participants
This PR expands the CLI with the following commands:
I think the purpose of these commands is pretty self-explanatory.
The actual output format of these commands was chosen according to fairly subjective criteria for utility + completeness + compactness. I will welcome any suggestions for change.
There is a beginning of a unit test there, nothing more in terms of automated testing. I am finding myself short of time for this lately, so I thought I'd pass it off to someone else at this point.
Related to: #826 #827 #831