Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat(hugr-cli)!: move mermaid to own sub-command #1390

Merged
merged 7 commits into from
Aug 2, 2024
Merged

feat(hugr-cli)!: move mermaid to own sub-command #1390

merged 7 commits into from
Aug 2, 2024

Conversation

ss2165
Copy link
Member

@ss2165 ss2165 commented Aug 2, 2024

drive-by replace clap_stdin with clio as that's that clap recommends, and it comes with stdout too.

BREAKING CHANGE: Cli validate command no longer has a mermaid option, use mermaid sub-command instead.

@ss2165 ss2165 requested a review from a team as a code owner August 2, 2024 10:18
@ss2165 ss2165 requested review from doug-q and removed request for a team August 2, 2024 10:18
doug-q
doug-q reviewed Aug 2, 2024
View reviewed changes
Copy link
Collaborator

@doug-q doug-q left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't it make sense to validate the Hugr AND mermaid-print it? I would not be surprised if mermaid-printing an invalid Hugr caused a panic. Will the lack of update_validate to replace OpaqueOp with ExtensionOp and do extension inference(I think? not sure of the state of both of these) cause problems?

@ss2165
Copy link
Member Author

ss2165 commented Aug 2, 2024

Yes perhaps, the change is because validate --mermaid --no-validate is just a confusing command to me.
I can common up the validation so mermaid also calls it, and add the --no-validate back to the mermaid command. Does that suit?

Base automatically changed from ss/package-cli to main August 2, 2024 10:54
@aborgna-q
Copy link
Collaborator

I would not be surprised if mermaid-printing an invalid Hugr caused a panic.

It shouldn't, as long as the underlying portgraph is working correctly.
I think not having rendering fail for unrelated errors is a good thing.

@ss2165 ss2165 changed the title feat(hugr-cli)!: move mermaid to own sub-command feat!(hugr-cli): move mermaid to own sub-command Aug 2, 2024
@ss2165
Copy link
Member Author

ss2165 commented Aug 2, 2024

I can common up the validation so mermaid also calls it,

In latest commit if you want to look at it. Though broadly I agree with @aborgna-q and would prefer to revert it.

@codecov Codecov
Copy link

codecov bot commented Aug 2, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 77.27273% with 10 lines in your changes missing coverage. Please review.

Project coverage is 87.64%. Comparing base (f78c0cc) to head (2f7034e).

Files Patch % Lines
hugr-cli/src/mermaid.rs 50.00% 2 Missing and 3 partials ⚠️
hugr-cli/src/lib.rs 82.35% 2 Missing and 1 partial ⚠️
hugr-cli/src/validate.rs 86.66% 0 Missing and 2 partials ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1390      +/-   ##
==========================================
- Coverage   87.64%   87.64%   -0.01%     
==========================================
  Files         117      118       +1     
  Lines       20329    20340      +11     
  Branches    18024    18035      +11     
==========================================
+ Hits        17818    17826       +8     
+ Misses       1725     1724       -1     
- Partials      786      790       +4
Flag Coverage Δ
python 91.67% <ø> (ø)
rust 87.12% <77.27%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ss2165 ss2165 changed the title feat!(hugr-cli): move mermaid to own sub-command feat!(hugr-cli): move mermaid to own sub-command Aug 2, 2024
@ss2165 ss2165 changed the title feat!(hugr-cli): move mermaid to own sub-command feat(hugr-cli)!: move mermaid to own sub-command Aug 2, 2024
@doug-q
Copy link
Collaborator

doug-q commented Aug 2, 2024

I would not be surprised if mermaid-printing an invalid Hugr caused a panic.

It shouldn't, as long as the underlying portgraph is working correctly. I think not having rendering fail for unrelated errors is a good thing.

This is convincing. My preference is an option to pass --validate to mermaid. I am partly motivated by wanting to have this option in other cli tools.

@ss2165 ss2165 requested a review from doug-q August 2, 2024 11:22
doug-q
doug-q approved these changes Aug 2, 2024
View reviewed changes
Copy link
Collaborator

@doug-q doug-q left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice, thank you.

@ss2165 ss2165 added this pull request to the merge queue Aug 2, 2024
Merged via the queue into main with commit 77795b9 Aug 2, 2024
20 checks passed
@ss2165 ss2165 deleted the ss/cli-merm branch August 2, 2024 12:54
This was referenced Aug 2, 2024
Merged
Merged
github-merge-queue bot pushed a commit that referenced this pull request Aug 12, 2024
@hugrbot @croyzor
chore: release (#1376)
8054c28 
## 🤖 New release
* `hugr`: 0.10.0 -> 0.11.0
* `hugr-core`: 0.7.0 -> 0.8.0
* `hugr-passes`: 0.6.2 -> 0.7.0
* `hugr-cli`: 0.3.0 -> 0.4.0

<details><summary><i><b>Changelog</b></i></summary><p>

## `hugr`
<blockquote>

## 0.11.0 (2024-08-12)

### Bug Fixes

- [**breaking**] BasicBlockExits should not be `OpTag::DataflowParent`
([#1409](#1409))

### Documentation

- Clarify CustomConst::equal_consts
([#1396](#1396))

### Features

- [**breaking**] Serialised extensions
([#1371](#1371))
- Serialised standard extensions
([#1377](#1377))
- [**breaking**] Update remaining builder methods to "infer by default"
([#1386](#1386))
- Add Eq op to logic extension
([#1398](#1398))
- Improve error message on failed custom op validation
([#1416](#1416))
- [**breaking**] `Extension` requires a version
([#1367](#1367))
</blockquote>

## `hugr-core`
<blockquote>

## 0.8.0 (2024-08-12)

### Bug Fixes

- [**breaking**] BasicBlockExits should not be `OpTag::DataflowParent`
([#1409](#1409))

### Documentation

- Clarify CustomConst::equal_consts
([#1396](#1396))

### Features

- [**breaking**] `Extension` requires a version
([#1367](#1367))
- [**breaking**] Serialised extensions
([#1371](#1371))
- Serialised standard extensions
([#1377](#1377))
- [**breaking**] Update remaining builder methods to "infer by default"
([#1386](#1386))
- Add Eq op to logic extension
([#1398](#1398))
- Improve error message on failed custom op validation
([#1416](#1416))
</blockquote>

## `hugr-passes`
<blockquote>

## 0.7.0 (2024-08-12)

### Features

- [**breaking**] `Extension` requires a version
([#1367](#1367))
</blockquote>

## `hugr-cli`
<blockquote>

## 0.4.0 (2024-08-12)

### Features

- Serialised standard extensions
([#1377](#1377))
- Validate with extra extensions and packages
([#1389](#1389))
- [**breaking**] Move mermaid to own sub-command
([#1390](#1390))
</blockquote>


</p></details>

---
This PR was generated with
[release-plz](https://github.com/MarcoIeni/release-plz/).

---------

Co-authored-by: Craig Roy <[email protected]>
github-merge-queue bot pushed a commit that referenced this pull request Aug 12, 2024
@hugrbot
chore(main): release hugr-py 0.6.0 (#1379)
937883d 
🤖 I have created a release *beep* *boop*
---


##
[0.6.0](hugr-py-v0.5.0...hugr-py-v0.6.0)
(2024-08-12)


### ⚠ BREAKING CHANGES

* **hugr-py:** Moved `hugr.get_serialization_version` to
`hugr.serialization.serial_hugr.serialization_version`
* **hugr-cli:** Cli validate command no longer has a mermaid option, use
`mermaid` sub-command instead.
* `TypeDefBound` uses struct-variants for serialization. `SignatureFunc`
now has variants for missing binary functions, and serializes in to a
new format that indicates expected binaries.

### Features

* `Package` pydantic model for modules + extensions
([#1387](#1387))
([68cfac5](68cfac5)),
closes [#1358](#1358)
* Define `Const` inline by default, and add a parameter to change the
parent ([#1404](#1404))
([3609736](3609736))
* **hugr-cli:** move mermaid to own sub-command
([#1390](#1390))
([77795b9](77795b9))
* **hugr-py:** add type_bound method to `Type`
([#1410](#1410))
([bd5ba47](bd5ba47)),
closes [#1365](#1365)
* **hugr-py:** Allow defining functions, consts, and aliases inside DFGs
([#1394](#1394))
([d554072](d554072))
* **hugr-py:** Reexport commonly used classes from the package root
([#1393](#1393))
([69925d0](69925d0))
* **py:** `Hugr.to_json` and `.load_json` helpers
([#1403](#1403))
([e7f9f4c](e7f9f4c))
* **py:** Allow pre-declaring a `Function`'s output types
([#1417](#1417))
([fa0f5a4](fa0f5a4))
* **py:** implement `iter` on `ToNode`
([#1399](#1399))
([e88910b](e88910b))
* **py:** Parametric int type helper, and arbitrary width int constants
([#1406](#1406))
([abd70c9](abd70c9))
* Serialised extensions
([#1371](#1371))
([31be204](31be204))


### Bug Fixes

* **py:** `Hugr.__iter__` returning `NodeData | None` instead of `Node`s
([#1401](#1401))
([c134584](c134584))
* **py:** Set output cont for Conditionals
([#1415](#1415))
([67bb8a0](67bb8a0))


### Documentation

* **hugr-py:** expand toctree
([#1411](#1411))
([aa81c9a](aa81c9a))
* **hugr-py:** remove multiversion + add justfile command
([#1381](#1381))
([dd1dc48](dd1dc48))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
@hugrbot hugrbot mentioned this pull request Aug 13, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@doug-q doug-q doug-q approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ss2165 @aborgna-q @doug-q