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

Don't automatically copy python objects into javascript #1152

Merged
merged 15 commits into from Jan 22, 2021

Conversation

hoodmane
Copy link
Member

@hoodmane hoodmane commented Jan 18, 2021

This implements (the most important change suggested in) #900. This should also make the custom globals / locals feature of eval_code implemented in #1106 work significantly better.

…oxy.shallowCopyToJavascript and deepCopyToJavascript apis
@hoodmane hoodmane marked this pull request as draft January 18, 2021 01:44
@hoodmane hoodmane marked this pull request as ready for review January 18, 2021 22:06
Copy link
Contributor

@phorward phorward left a comment

Choose a reason for hiding this comment

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

Hello @hoodmane, thanks for the Pull Request. From the code base this looks fine to me. Can you please shortly explain the changes in the test suite?

packages/matplotlib/test_matplotlib.py Show resolved Hide resolved
@dalcde
Copy link
Contributor

dalcde commented Jan 21, 2021 via email

@hoodmane
Copy link
Member Author

Yeah it would work, I opted for ; pass because I think that is a little more conspicuous.

@phorward phorward merged commit 6a44233 into pyodide:master Jan 22, 2021
@phorward
Copy link
Contributor

phorward commented Jan 22, 2021

@hoodmane: It seems that we now have a regression with test_console.py and your change, can you please take a look at it? I don't know how to fix it quickly.

___________________________ test_completion[firefox] ___________________________
[gw2] linux -- Python 3.8.2 /usr/local/bin/python

selenium = <conftest.FirefoxWrapper object at 0x7f6529803a00>
safe_selenium_sys_redirections = None

    def test_completion(selenium, safe_selenium_sys_redirections):
        selenium.run(
            """
        from pyodide import console
    
        shell = console.InteractiveConsole()
        """
        )
    
>       assert selenium.run("shell.complete('a')") == [
            [
                "and ",
                "as ",
                "assert ",
                "async ",
                "await ",
                "abs(",
                "all(",
                "any(",
                "ascii(",
            ],
            0,
        ]
E       AssertionError: assert [None, 0] == [['and ', 'as...bs(', ...], 0]
E         At index 0 diff: None != ['and ', 'as ', 'assert ', 'async ', 'await ', 'abs(', 'all(', 'any(', 'ascii(']
E         Full diff:
E           [
E         +  None,
E         -  ['and ',
E         -   'as ',
E         -   'assert ',
E         -   'async ',
E         -   'await ',
E         -   'abs(',
E         -   'all(',
E         -   'any(',
E         -   'ascii('],
E            0,
E           ]

src/tests/test_console.py:201: AssertionError

Next time it would be better to first merge master into this branch, and re-test it first.

phorward added a commit that referenced this pull request Jan 22, 2021
@phorward
Copy link
Contributor

@hoodmane can you please investigate? I think the problem is in the conversion of Tuples, which is the case here.

pyodide.runPython("from pyodide import console; shell = console.InteractiveConsole(); shell.complete('a')")

returns Array [ Proxy, 0 ], which seems to be invalid, as it should be Array [ Array ['and ', 'as ', 'assert ', 'async ', 'await ', 'abs(', 'all(', 'any(', 'ascii('], 0 ].

@phorward
Copy link
Contributor

@hoodmane I'm sorry I reverted your Pull Request because after a 3 hour examination of the problem I wasn't able to get into a solution. Maybe you should take a look on it. Can you then please re-open the Pull Request again, as I don't have a possibility to do it here.

@hoodmane
Copy link
Member Author

This PR currently is a bit bad for usability, it needs some supporting changes to pyproxy to make it a bit easier to use. I was thinking the right approach would be to merge this as-is and then preferably fix the ergonomics problems before the next release, but I could see adding more to this PR.
The tests can be fixed by turning:

        shell.complete('a = 0 ; print.__g')

into

        [completions, start] = shell.complete('a = 0 ; print.__g')
        [tuple(completions), start]

@hoodmane
Copy link
Member Author

Oh also the docs on type conversions and the changelog should be updated.

@hoodmane
Copy link
Member Author

Presumably implementing toJSON on pyproxy would make the tests pass more painlessly.

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

3 participants