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

Rename callSyncifying to callPromising, add callWithOptions #4608

Merged
merged 5 commits into from Apr 22, 2024

Conversation

hoodmane
Copy link
Member

@hoodmane hoodmane commented Mar 12, 2024

Since we renamed syncify the method name callSyncifying doesn't make much sense anymore. It is implemented in terms of a so-called promising wasm call, so this lines up with that. OTOH:

  • Nobody is likely to know what that refers to
  • most people won't need this anyways
  • it does return a promise so the name is somewhat descriptive
  • the main point of it is to call a sync function and enable stack switching
  • so maybe something like suspendableCall would be more appropriate?

We now have three boolean parameters for a Js-to-Python call:

  1. kwargs
  2. promising
  3. relaxed

So we'd need 8 functions to cover all combinations of these. Currently we only have 6 of these. Rather than adding the two remaining combinations which will have annoying names, I added callOptions which takes an options argument as the first argument. Despite the fact that options usually go as the last argument, I think it makes sense to use the first argument for this so that all remaining args are passed on to Python. There's also a suggestion of adding a timeout parameter #4537 which this would reasonably accomodate.

Is callOptions a good name? @rth @ryanking13 anyone have thoughts on names (or anything else)?

Checklist

  • Add a CHANGELOG entry
  • Add / update tests
  • Add new / update outdated documentation

Since we renamed `syncify` the method name `callSyncifying` doesn't make much
sense anymore. It is implemented in terms of a so-called `promising` wasm call,
so this lines up with that. OTOH:

* Nobody is likely to know what that refers to
* most people won't need this anyways
* it does return a promise so the name is somewhat descriptive
* the main point of it is to call a sync function and enable stack switching
* so maybe something like `suspendableCall` would be more appropriate?

We now have three boolean parameters for a Js-to-Python call:

1. kwargs
2. promising
3. relaxed

So we'd need 8 functions to cover all combinations of these. Currently we only
have 6 of these. Rather than adding the two remaining combinations which will
have annoying names, I added `callOptions` which takes an options argument as
the first argument. Despite the fact that options usually go as the last
argument, I think it makes sense to use the first argument for this so that all
remaining args are passed on to Python. There's also a suggestion of adding a
timeout parameter pyodide#4537 which this would reasoanbly accomodate.

Is `callOptions` a good name?
@hoodmane hoodmane changed the title Rename callSyncifying to callPromising, add callOpts Rename callSyncifying to callPromising, add callOptions Mar 12, 2024
Copy link
Member

@ryanking13 ryanking13 left a comment

Choose a reason for hiding this comment

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

Thanks @hoodmane!

Naming is hard and it's especially hard to come up with an intuitive name for something as technically complex as this.

My feeling for callPromising and callOptions is:

  • callPromising: The relationship to run_sync doesn't seem to be obvious, at least from the name. If what we expect is to use callPromising with run_sync, wouldn't it be nice to have a little more similarity in the name?
  • callOptions: callOptions sounds like an object that contains the parameters for the function call, rather than a method name. Maybe callWithOptions?

@hoodmane
Copy link
Member Author

The relationship to run_sync doesn't seem to be obvious, at least from the name. If what we expect is to use callPromising with run_sync, wouldn't it be nice to have a little more similarity in the name?

Yes, I suppose we should brainstorm what that would be... As you've probably noticed, I usually name things by starting with something bad and begging @rth or you to come up with something better.

Maybe callWithOptions?

That sounds good to me.

@hoodmane hoodmane changed the title Rename callSyncifying to callPromising, add callOptions Rename callSyncifying to callPromising, add callWithOptions Apr 22, 2024
@hoodmane hoodmane merged commit 33a24c2 into pyodide:main Apr 22, 2024
6 of 8 checks passed
@hoodmane hoodmane deleted the callPromising branch April 22, 2024 12:17
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.

None yet

2 participants