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

Add context manager to enable/disable patching #48

Open
wants to merge 1 commit into
base: branch-24.06
Choose a base branch
from

Conversation

rjzamora
Copy link
Member

Motivated by @vyasr 's suggestion in #46

Adds a simple context/utility to toggle patching behavior. This should make it easier to quickly test if patching is causing a hypothetical bug in the future.

@rjzamora rjzamora added improvement Improves an existing functionality non-breaking Introduces a non-breaking change labels May 22, 2024
@rjzamora rjzamora self-assigned this May 22, 2024
@rjzamora rjzamora requested review from vyasr and a team as code owners May 22, 2024 19:20
Copy link
Member

@pentschev pentschev 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 idea and it generally looks good to me. However, I don't feel experienced enough with the patching mechanism and would prefer to have @vyasr reviewing.

Thanks @rjzamora for working on this, I can see this being very useful for debugging issues in the future. 🙂

Copy link
Contributor

@vyasr vyasr left a comment

Choose a reason for hiding this comment

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

Instead of rewriting the environment each time in the CM, can we load it once at the module level and store it in a module-scope var that is overwritten temporarily in the CM? Modifying the environment each time feels pretty heavyweight.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improves an existing functionality non-breaking Introduces a non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants