-
Notifications
You must be signed in to change notification settings - Fork 7
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: hugr binary cli tool #1096
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1096 +/- ##
==========================================
+ Coverage 86.66% 86.73% +0.06%
==========================================
Files 88 90 +2
Lines 18467 18501 +34
Branches 18074 18108 +34
==========================================
+ Hits 16005 16047 +42
+ Misses 1621 1615 -6
+ Partials 841 839 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
Nice coverage. I have a suggestion inline but I do not feel strongly if you disagree.
hugr/src/cli.rs
Outdated
pub const VALID_PRINT: &str = "HUGR valid!"; | ||
|
||
/// Run the HUGR cli and validate against an extension registry. | ||
pub fn run(registry: &ExtensionRegistry) -> Result<(), Box<dyn std::error::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.
I would prefer to expose pub CmdLineArgs
and either take it as a parameter here, or put this function on impl CmdLineArgs
.
This moves the CmdLineArgs::parse()
into the caller, which allows callers to add (and handle) options, embed as a SubCommand
etc.
hugr/tests/cli.rs
Outdated
|
||
#[fixture] | ||
fn test_hugr_file(test_hugr_string: String) -> NamedTempFile { | ||
// TODO use proptests? |
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.
For generating the Hugr? That's a long way off, we need to generate valid sub-properties then wire them together into a Hugr. I don't think it would be useful here, we are already testing serialisation elsewhere.
Does the cli need access to non-public stuff? It would be cleaner to have it as a separate subcrate instead; Add a [[bin]]
name = "hugr"
path = "src/main.rs" This way we don't need to add a new feature that's useless to lib users. |
The reasoning, which should be in the commit message, is to make I disagree it's useless to lib users. |
After some discussion; if we want to be able to do I still think the library should go in a subcrate. The We can merge this PR as-is, and I'll propose the subcrate in a separate issue. |
## 🤖 New release * `hugr`: 0.4.0 -> 0.5.0 * `hugr-cli`: 0.1.0 * `hugr-core`: 0.1.0 * `hugr-passes`: 0.1.0 <details><summary><i><b>Changelog</b></i></summary><p> ## `hugr` <blockquote> ## 0.5.0 (2024-05-29) ### Bug Fixes - Missing re-exports in `hugr::hugr` ([#1127](#1127)) - Set initial version of hugr-core to 0.1.0 ([#1129](#1129)) ### Features - [**breaking**] Remove `PartialEq` impl for `ConstF64` ([#1079](#1079)) - [**breaking**] Allow "Row Variables" declared as List<Type> ([#804](#804)) - Hugr binary cli tool ([#1096](#1096)) - [**breaking**] Move passes from `algorithms` into a separate crate ([#1100](#1100)) - [**breaking**] Disallow nonlocal value edges into FuncDefn's ([#1061](#1061)) - [**breaking**] Move cli in to hugr-cli sub-crate ([#1107](#1107)) - Add verbosity, return `Hugr` from `run`. ([#1116](#1116)) - Unseal and make public the traits `HugrInternals` and `HugrMutInternals` ([#1122](#1122)) ### Refactor - [**breaking**] No Ports in TypeRow ([#1087](#1087)) - Add a `hugr-core` crate ([#1108](#1108)) </blockquote> ## `hugr-core` <blockquote> ## 0.1.0 (2024-05-29) ### Bug Fixes - Set initial version of hugr-core to 0.1.0 ([#1129](#1129)) ### Features - [**breaking**] Move cli in to hugr-cli sub-crate ([#1107](#1107)) - Make internals of int ops and the "int" CustomType more public. ([#1114](#1114)) - Unseal and make public the traits `HugrInternals` and `HugrMutInternals` ([#1122](#1122)) ### Refactor - Add a `hugr-core` crate ([#1108](#1108)) </blockquote> </p></details> --- This PR was generated with [release-plz](https://github.com/MarcoIeni/release-plz/). --------- Co-authored-by: Agustin Borgna <[email protected]>
Closes #1095
currently validates against std extensions
can read from stdin with
echo "<json>" | hugr -
binary depends on optional feature to limit dependencies, but not put in sub crate to allow
cargo install hugr
add integration testing of binary behaviour
based on https://docs.rs/assert_cmd/2.0.14/assert_cmd/cmd/struct.Command.html#method.write_stdin