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

Implement Object.get and Object.set #81

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

Conversation

qwenger
Copy link
Collaborator

@qwenger qwenger commented Apr 8, 2022

Implement Object.get and Object.set using the same ideas as Context.get and Context.set.

Currently it is very tedious to get or set an object property. When the object is only known via its Python wrapper, one has to first rebind it to the global context using Context.set and then access it with Context.get. With this, one could directly access or set the property via the Python wrapper.

The json() method is not sufficient for this, as it does not expose properties from the prototype.

Discussion:

  • What about merging these methods with those of Context, as in, treating the Context as an Object? (Respectively, remove the get, set, etc. methods of Context, add a generic get_global or similar to get the global object from the context)

Further discussion (for the future?):

  • What about add_callable? Should Object also support it? Should we merge set and add_callable?
  • What about having a more transparent interface between JS and Python? Like being able to access properties of Object with the standard dot syntax (and/or square brackets lookup?)?

Copy link
Owner

@PetterS PetterS left a comment

Choose a reason for hiding this comment

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

Looks useful

if (ret == 1) {
Py_RETURN_NONE;
} else {
return NULL;
Copy link
Owner

Choose a reason for hiding this comment

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

Need an error message if python_to_quickjs_possible fails.

The unit tests should also cover the failure cases.

Copy link
Collaborator Author

@qwenger qwenger Apr 11, 2022

Choose a reason for hiding this comment

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

Agreed (though this code is copied over / modified from context_set, so that should be the case there too).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually, python_to_quickjs_possible does set an error message when failing, so that's fine. Still agreeing on the need for unit tests.

@PetterS
Copy link
Owner

PetterS commented Apr 11, 2022

When I originally created this library, I did the bare minimum for it to be useful in production. The API was deliberately kept small to reduce the likelihood of bugs. However, I don't use it in production any more, so if you wish to make it easier to use, we can do so. Just make sure everything including failures/exceptions is covered by tests.

What about add_callable? Should Object also support it? Should we merge set and add_callable?

May be a good idea to merge them, yes.

What about having a more transparent interface between JS and Python? Like being able to access properties of Object with the standard dot syntax (and/or square brackets lookup?)?

I like this. Can be implemented in Python code as a convenience.

@qwenger
Copy link
Collaborator Author

qwenger commented Apr 11, 2022

What about add_callable? Should Object also support it? Should we merge set and add_callable?

May be a good idea to merge them, yes.

Could you please have a look at #82? It is an alternative proposal that goes further by merging set and add_callable and moving the get/set methods to Object.

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