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.

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

Rename Region views #347

Merged
merged 3 commits into from
Aug 7, 2023
Merged

Rename Region views #347

merged 3 commits into from
Aug 7, 2023

Conversation

lmondada
Copy link
Contributor

@lmondada lmondada commented Aug 3, 2023

I propose the following renaming

  • FlatRegionView -> SiblingGraph
  • RegionView -> DescendantsGraph
  • Region -> HierarchyView

I've reserved the term View for the trait rather than the structs as that is closer to our use of the word elsewhere. Happy to discuss other opinions.

In addition to being more descriptive, this will also allow us to make the complementarity with #336 more obvious.

@lmondada lmondada requested a review from ss2165 August 3, 2023 12:00
@lmondada
Copy link
Contributor Author

lmondada commented Aug 3, 2023

Credits to @acl-cqc for the suggestions!

Copy link
Member

@ss2165 ss2165 left a comment

Choose a reason for hiding this comment

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

I like the names, and the reserving of View for traits. Just a suggestion on the module name.

@@ -1,14 +1,16 @@
//! Region-level views of a HUGR.
Copy link
Member

Choose a reason for hiding this comment

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

maybe the file/module should be called views.rs?

Copy link
Contributor Author

@lmondada lmondada Aug 3, 2023

Choose a reason for hiding this comment

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

I would like that, but there is already the view.rs file in the same folder that contains HugrView etc.

I could make a views folder with

  • views/hugr.rs
  • views/hierarchy.rs

in it?

Copy link
Member

Choose a reason for hiding this comment

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

or just go with hierarchical_views

@lmondada lmondada requested a review from ss2165 August 7, 2023 12:43
Copy link
Member

@ss2165 ss2165 left a comment

Choose a reason for hiding this comment

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

👍 thanks

@lmondada lmondada enabled auto-merge August 7, 2023 12:44
@lmondada lmondada added this pull request to the merge queue Aug 7, 2023
Merged via the queue into main with commit 24fef6a Aug 7, 2023
@lmondada lmondada deleted the refactor/rename_regions branch August 7, 2023 12:48
@lmondada lmondada mentioned this pull request Aug 7, 2023
cqc-alec pushed a commit that referenced this pull request Aug 7, 2023
* Rename `Region` views

* Rename hierarchy -> hierarchical_views
github-merge-queue bot pushed a commit that referenced this pull request Aug 8, 2023
* Add simpler variant of Resource.add_op_custom_sig().

* Use Resource.add_op_custom_sig_simple() where possible.

* Fix typo in spec.

* Define SignatureError::InvalidTypeArgs.

* Define HashableType::OpError.

* Define TypeArgError::InvalidValue.

* Implement Arithmetic Resource.

* Use constants rather than functions for type IDs.

* Move arithmetic.rs to arithmetic/mod.rs.

* Add method to create a resource with requirements.

* Add method to create a ResourceSet from some resources.

* Refactor into separate modules.

* Rename `Region` views (#347)

* Rename `Region` views

* Rename hierarchy -> hierarchical_views

* separate TypeParam hierarchy (#364)

* separate TypeParam hierarchy

Closes #359

* address review comments

* move the contents of typecheck.rs to values.rs

* more review comments

* cherry-pick CustomTypeArg from #335

* define TypeTag::contains and use in arg check

* Post-merge fixup.

* Remove INT_PARAM constant.

* Use constants for resource IDs, and settle on lowercase.

* Remove HashableType::OpError and just use String (for now).

* Make SignatureError::InvalidTypeArgs a constant.

* Use utility function collect_array().

---------

Co-authored-by: Luca Mondada <[email protected]>
Co-authored-by: Seyon Sivarajah <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants