CLI improvements #1554
CLI improvements #1554
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?
| 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