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

Switch PyProxy.toString from repr to str #4247

Merged
merged 5 commits into from Oct 23, 2023
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions docs/project/changelog.md
Expand Up @@ -23,6 +23,11 @@ myst:
value and leaked memory.
{pr}`4236`

- {{ Breaking }} `PyProxy.toString` now calls `str` instead of `repr`. For now
you can opt into the old behavior by passing `pyproxyToStringRepr: true` to
`loadPyodide`, but this may be removed in the future.
{pr}`4247`

### Packages

- Added `river` version 0.19.0 {pr}`4197`
Expand Down
2 changes: 2 additions & 0 deletions docs/usage/type-conversions.md
Expand Up @@ -126,6 +126,7 @@ returned. The following operations are currently supported on a {py:class}`~pyod
| Python | JavaScript |
| ---------------------------------- | --------------------------------- |
| `str(proxy)` | `x.toString()` |
| `repr(proxy)` | `x.toString()` |
| `proxy.foo` | `x.foo` |
| `proxy.foo = bar` | `x.foo = bar` |
| `del proxy.foo` | `delete x.foo` |
Expand Down Expand Up @@ -235,6 +236,7 @@ operations are more cumbersome on a {js:class}`~pyodide.ffi.PyProxy` than on a

| JavaScript | Python |
| ----------------------------------- | ------------------- |
| `proxy.toString()` | `str(x)` |
| `foo in proxy` | `hasattr(x, 'foo')` |
| `proxy.foo` | `x.foo` |
| `proxy.foo = bar` | `x.foo = bar` |
Expand Down
9 changes: 8 additions & 1 deletion src/core/pyproxy.c
Expand Up @@ -301,13 +301,20 @@ pyproxy_getflags(PyObject* pyobj)
// logic is very boilerplatey, so there isn't much surprising code hidden
// somewhere else.

EMSCRIPTEN_KEEPALIVE
bool compat_to_string_repr = false;

EMSCRIPTEN_KEEPALIVE JsVal
_pyproxy_repr(PyObject* pyobj)
{
PyObject* pyrepr = NULL;
JsVal jsrepr = JS_NULL;

pyrepr = PyObject_Repr(pyobj);
if (compat_to_string_repr) {
pyrepr = PyObject_Repr(pyobj);
} else {
pyrepr = PyObject_Str(pyobj);
}
FAIL_IF_NULL(pyrepr);
jsrepr = python2js_val(pyrepr);

Expand Down
4 changes: 4 additions & 0 deletions src/core/pyproxy.ts
Expand Up @@ -628,6 +628,10 @@ export class PyProxy {
let ptrobj = _getPtr(this);
return __pyproxy_type(ptrobj);
}
/**
* Returns `str(o)` (unless `pyproxyToStringRepr: true` was passed to
* :js:func:`loadPyodide` in which case it will return `repr(o)`)
*/
toString(): string {
let ptrobj = _getPtr(this);
let result;
Expand Down
4 changes: 4 additions & 0 deletions src/js/api.ts
Expand Up @@ -37,6 +37,10 @@ API.runPythonInternal = function (code: string): any {
return API._pyodide._base.eval_code(code, API.runPythonInternal_dict);
};

API.setPyProxyToStringMethod = function (useRepr: boolean): void {
Module.HEAP8[Module._compat_to_string_repr] = +useRepr;
};

/** @private */
export type NativeFS = {
syncfs: () => Promise<void>;
Expand Down
9 changes: 9 additions & 0 deletions src/js/pyodide.ts
Expand Up @@ -314,6 +314,12 @@ export async function loadPyodide(
* while Pyodide bootstraps itself.
*/
packages?: string[];
/**
* Opt into the old behavior where PyProxy.toString calls `repr` and not
* `str`.
* @deprecated
*/
pyproxyToStringRepr?: boolean;
/**
* @ignore
*/
Expand Down Expand Up @@ -395,6 +401,9 @@ export async function loadPyodide(
if (Module.exited) {
throw Module.exited.toThrow;
}
if (options.pyproxyToStringRepr) {
API.setPyProxyToStringMethod("repr");
hoodmane marked this conversation as resolved.
Show resolved Hide resolved
}

if (API.version !== version) {
throw new Error(
Expand Down
1 change: 1 addition & 0 deletions src/js/types.ts
Expand Up @@ -279,6 +279,7 @@ export interface API {
NoGilError: any;
errorConstructors: Map<string, ErrorConstructor>;
deserializeError: (name: string, message: string, stack: string) => Error;
setPyProxyToStringMethod: (useRepr: boolean) => void;

_pyodide: any;
pyodide_py: any;
Expand Down
17 changes: 17 additions & 0 deletions src/tests/test_pyproxy.py
Expand Up @@ -65,6 +65,23 @@ def get_value(self, value):
}.difference(selenium.run_js("return f_props")) == set()


@run_in_pyodide
def test_pyproxy_tostring(selenium):
from pathlib import Path

from pyodide.code import run_js
from pyodide_js._api import setPyProxyToStringMethod

pyproxy_to_string = run_js("(e) => e.toString()")

p = Path("a/b/c")
assert pyproxy_to_string(p) == str(p)
setPyProxyToStringMethod(True)
assert pyproxy_to_string(p) == repr(p)
setPyProxyToStringMethod(False)
assert pyproxy_to_string(p) == str(p)


def test_del_builtin(selenium):
msg = "NameError"
with pytest.raises(selenium.JavascriptException, match=msg):
Expand Down