Skip to content

Conversation

@mstechly
Copy link
Contributor

@mstechly mstechly commented Nov 5, 2024

Description

Adds support for repeated structures and a couple of other minor features.
For tests to pass it would require PsiQ/qref#124 to go in first an QREF to be released.

There's a couple of things left:

  • Documentation
  • Some minor test cases are missing
  • Moving _evaluate_and_define_functions in an appropriate place.

Please verify that you have completed the following steps

  • I have self-reviewed my code.
  • I have included test cases validating introduced feature/fix.
  • I have updated documentation.

@cla-bot cla-bot bot added the cla-signed label Nov 5, 2024
Copy link
Contributor

@dexter2206 dexter2206 left a comment

Choose a reason for hiding this comment

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

Overall looks good, and I only left minor suggestions, with a following caveat:

After reviewing this and the QREF's counterpart of this PR I'm more and more convinced that it would be better if we had a separate class representing "loop" or a "routine with repetitions" (or at least if we had it in Bartiq). This is because a lot of the logic we perform for validation and dispatching (with ifs) could be neatly hidden. For instance, we verify if routine with repetition has one child - but we could have an object that HAS to have one child (which would be validated by Pydantic). Anyway, we can continue with the current solution and later refactor it if we have some time to do so.

@mstechly mstechly merged commit 57312a1 into main Nov 7, 2024
@mstechly mstechly deleted the repetitive-structures branch November 7, 2024 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

2 participants