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

Make Jsproxy getattr and dir use GenericGetAttr and object.__dir__ #1017

Merged
merged 13 commits into from Jan 5, 2021

Conversation

hoodmane
Copy link
Member

@hoodmane hoodmane commented Jan 2, 2021

Before this, the JsProxy getattr method explicitly compared the attribute name against a list of strings to decide whether to pull the attribute off of a python object. dir only returned the keys of the wrapped javascript object, not the extra keys coming from python.

Now I use PyObject_GenericGetAttr so that if JsProxy.__dict__ has a key, we return the associated value, and dir returns the merged list of python and javascript keys.

This shadows more attributes of the javascript object, but they are all names with __underscores__ so I think it's okay.

@hoodmane
Copy link
Member Author

hoodmane commented Jan 2, 2021

Similar changes are included in #972 but I figured this was worth separating off.

@rth rth mentioned this pull request Jan 4, 2021
src/type_conversion/jsproxy.c Outdated Show resolved Hide resolved
@rth
Copy link
Member

rth commented Jan 4, 2021

Now I use PyObject_GenericGetAttr so that if JsProxy.dict has a key, we return the associated value, and dir returns the merged list of python and javascript keys.

Might be good to add a test for it?

Also could you rename this PR to something more specific than "redo"?

@hoodmane hoodmane changed the title redo Jsproxy getattr and dir Make Jsproxy getattr and dir use GenericGetAttr and object.__dir__ Jan 4, 2021
@rth
Copy link
Member

rth commented Jan 5, 2021

We should add a changelog entry, otherwise LGTM.

@hoodmane
Copy link
Member Author

hoodmane commented Jan 5, 2021

Okay updated changelog. A little hard to describe because this is more of a backend thing, but it does have useful visible effects on the user like typeof and new now appear in dir(jsproxy).

@rth rth merged commit 34af5c0 into pyodide:master Jan 5, 2021
@hoodmane hoodmane deleted the jsproxy-getattr branch January 5, 2021 14:10
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