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

Remove deepcopy #173

Merged
merged 1 commit into from
Jun 14, 2024
Merged

Remove deepcopy #173

merged 1 commit into from
Jun 14, 2024

Conversation

stympy
Copy link
Member

@stympy stympy commented Jun 11, 2024

Fixes #172

Copy link
Member

@joshuap joshuap left a comment

Choose a reason for hiding this comment

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

I'm lacking context on the substance of this change tbh, so it would be nice if @Kelvin4664 or @subzero10 could also review.

@Kelvin4664
Copy link
Collaborator

@joshuap We added deepcopy to fix #165, which is a far less critical bug than this one.
I think it makes sense to remove since that bug was a user error and we were only trying to make the system fail safe.

@Kelvin4664
Copy link
Collaborator

Also, i believe directly reverting that PR will be a better approach here. I have opened #174 for that. Someone take a look before i merge.

@stympy
Copy link
Member Author

stympy commented Jun 14, 2024

I think it makes sense to keep the removal of keys that are tuples, but the way that it was done could be improved (this PR), so I'm going to go with this to fix the issue, and we can revisit if necessary.

@stympy stympy merged commit 8642c7b into master Jun 14, 2024
19 checks passed
@subzero10
Copy link
Member

I think it makes sense to keep the removal of keys that are tuples

+1

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.

Cannot pickle TextIOWrapper
4 participants