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

avoid recursive_replace() in block_creation.py #16708

Merged
merged 1 commit into from
Nov 30, 2023

Conversation

arvidn
Copy link
Contributor

@arvidn arvidn commented Oct 27, 2023

Purpose:

It makes the code a bit more linear and avoids copying objects.
This is the only production use of recursive_replace(), and with this it could be turned into a test-only facility.

My long term intention is to make these types (FullBlock and all its members, transitively) not be dataclasses, but instead native types. This poses a challenge with all the places we use replace(). recursive_replace() is especially challenging to substitute.

This is a small step towards reducing the use of replace(). One possibility might be to have these objects be mutable, to cover some of the existing use of replace(), but not all.

… It makes the code a bit more linear and avoids copying objects
@arvidn arvidn requested a review from a team as a code owner October 27, 2023 13:28
@arvidn arvidn added the Changed Required label for PR that categorizes merge commit message as "Changed" for changelog label Oct 27, 2023
@arvidn arvidn requested a review from altendky November 25, 2023 15:11
Copy link
Contributor

@altendky altendky left a comment

Choose a reason for hiding this comment

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

Much better, thanks.

Copy link
Contributor

@wjblanke wjblanke left a comment

Choose a reason for hiding this comment

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

aok

@arvidn arvidn added the ready_to_merge Submitter and reviewers think this is ready label Nov 30, 2023
@cmmarslender cmmarslender merged commit 72028b7 into main Nov 30, 2023
254 of 255 checks passed
@cmmarslender cmmarslender deleted the block-creation-recursive-replace branch November 30, 2023 20:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changed Required label for PR that categorizes merge commit message as "Changed" for changelog ready_to_merge Submitter and reviewers think this is ready
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants