Skip to content

refactor: modify rewriter structure to allow method chaining and history tracking#237

Merged
Brendan-Reid1991 merged 231 commits intomainfrom
refactor_rewriters
Jul 2, 2025
Merged

refactor: modify rewriter structure to allow method chaining and history tracking#237
Brendan-Reid1991 merged 231 commits intomainfrom
refactor_rewriters

Conversation

@Brendan-Reid1991
Copy link
Copy Markdown
Contributor

Description

Refactor of the rewriter classes to be data classes instead of standard python classes.

  • WHY?
    • Users found that method chaining felt intuitive, so returning a new instance of the rewriter object allows this method of working.
  • HOW?
    • Rewriters are now data classes, and mutating methods return new instances of the Rewriter.
  • WHY? (again)
    • This is a much cleaner implementation and closer to the standard code quality existing in Bartiq.

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 Jul 1, 2025
Copy link
Copy Markdown
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.

Good stuff, I only found minor issues and left some improvement suggestions.

We talked with @Brendan-Reid1991 off the Github and agreed that maybe revert_to method is not how we want to interact with history. Therefore, all the comments I left regarding problems with this methods can be ignored.

Copy link
Copy Markdown
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.

LGTM!

For posterity: we agreed that factory functions for constructing rewriter can be implemented in another PR, all my other comments have been addressed.

@Brendan-Reid1991 Brendan-Reid1991 merged commit 7f69ef4 into main Jul 2, 2025
9 checks passed
@Brendan-Reid1991 Brendan-Reid1991 deleted the refactor_rewriters branch July 2, 2025 15:52
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.

5 participants