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 a global manager for configuring runtimes #141

Merged
merged 13 commits into from
Jul 23, 2024

Conversation

ryanking13
Copy link
Member

This adds a global config manager for pytest-pyodide.

This is mostly for use in pyodide, where we already modify browser flags in conftest.py. This PR aims to provide a more stable and public API to modify them.

Copy link
Member

@hoodmane hoodmane 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 not really sure how much of a difference this makes, we had a global config before, now we still have one. I guess as a signal instead of "this is just a hack" it says "we thought about it and we think this is the right hack".

def set_flag(self, runtime: RUNTIMES, flags: list[str]):
self.flags[runtime] = flags

def get_flag(self, runtime: RUNTIMES) -> list[str]:
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't this be get_flags / set_flags plural?

@ryanking13
Copy link
Member Author

I'm not really sure how much of a difference this makes, we had a global config before, now we still have one. I guess as a signal instead of "this is just a hack" it says "we thought about it and we think this is the right hack".

That is true, but at least we now provide a public API that is less likely to change. In the follow-up, I would like to put the values like pyodide_runtimes in this class too.

@ryanking13 ryanking13 merged commit d7da97c into pyodide:main Jul 23, 2024
16 checks passed
@ryanking13 ryanking13 deleted the global-state-manager2 branch July 23, 2024 11:51
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.

2 participants