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 loadPackagesFromImports from runPythonAsync #1538

Merged
merged 6 commits into from Apr 28, 2021

Conversation

hoodmane
Copy link
Member

Resolves #1496.

@rth
Copy link
Member

rth commented Apr 26, 2021

One one side I understand the motivation for removing it #1496 (comment) on the other function loadPackagesAndRunPythonAsync is not really readable IMO and it does look like we are using a fair amount of times (and aside for examples are never using this new version of runPythonAsync).

Maybe addition an optional parameter to runPythonAsync would be more readable?

@hoodmane
Copy link
Member Author

hoodmane commented Apr 26, 2021

it does look like we are using a fair amount of times

I just converted everything to the old version to save energy on converting them. I think in many places package loading is not actually needed. I can go through and replace with:

await pyodide.loadPackage("some_pkg"); 
await pyodide.runPythonAsync(code)

or with just runPythonAsync if nothing is actually being loaded.
I think this is much less surprising. I think it's better to just have separate functions do_a() and do_b() than to have do_b(do_a_also=True).

@rth
Copy link
Member

rth commented Apr 26, 2021

OK, then maybe we could remove loadPackagesAndRunPythonAsync as well? As you said calling it explicitly is just one extra line.

@rth rth merged commit ffa9915 into pyodide:main Apr 28, 2021
@hoodmane hoodmane deleted the run-python-async branch April 28, 2021 09:04
hamlet4401 pushed a commit to tytgatlieven/pyodide that referenced this pull request Jul 3, 2021
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.

Remove loadPackagesFromImports from runPythonAsync?
2 participants