ronantakizawa/codereview-bench huggingface dataset:
Repository: pyscript/pyscript
File: pyscriptjs/src/python/pyscript/__init__.py (Python)

Code before:
            f"<code>{name}</code> is deprecated. "
            "This is a private implementation detail of pyscript. "
            "You should not use it."
        )
        ns[name] = DeprecatedGlobal(name, obj, message)

    # these names are available as js.XXX
    for name in ["document", "console"]:
        obj = getattr(js, name)
        deprecate(name, obj, f"Please use <code>js.{name}</code> instead.")

    # PyScript is special, use a different message
    message = (
        "The <code>PyScript</code> object is deprecated. "
        "Please use <code>pyscript</code> instead."
    )
    ns["PyScript"] = DeprecatedGlobal("PyScript", PyScript, message)


def _run_pyscript(code, id=None):
    import __main__

    with _display_target(id):
        result = eval_code(code, globals=__main__.__dict__)
    # The output of `runPython` is wrapped inside an dict since a dict is not
    # thenable. This is so we do not accidentally `await` the result of theß
    # python execution, even if it's awaitable (Future, Task, etc.)
    return js.Object.new(result=result)


Review comment (suggestion):
```suggestion
    # thenable. This is so we do not accidentally `await` the result of the
```

Code after:
            f"<code>{name}</code> is deprecated. "
            "This is a private implementation detail of pyscript. "
            "You should not use it."
        )
        ns[name] = DeprecatedGlobal(name, obj, message)

    # these names are available as js.XXX
    for name in ["document", "console"]:
        obj = getattr(js, name)
        deprecate(name, obj, f"Please use <code>js.{name}</code> instead.")

    # PyScript is special, use a different message
    message = (
        "The <code>PyScript</code> object is deprecated. "
        "Please use <code>pyscript</code> instead."
    )
    ns["PyScript"] = DeprecatedGlobal("PyScript", PyScript, message)


def _run_pyscript(code: str, id: str = None) -> JsProxy:
    """Execute user code inside context managers.

    Uses the __main__ global namespace.

    The output is wrapped inside a JavaScript object since an object is not
    thenable. This is so we do not accidentally `await` the result of the python
    execution, even if it's awaitable (Future, Task, etc.)

    Parameters
    ----------
    code :
       The code to run

    id :
       The id for the default display target (or None if no default display target).

    Returns
    -------
        A Js Object of the form {result: the_result}
    """
    import __main__

    with _display_target(id):
        result = eval_code(code, globals=__main__.__dict__)

    return js.Object.new(result=result)
Repository: pyscript/pyscript
File: pyscriptjs/src/components/pyscript.ts (TypeScript)

Code before:
        // el.onclick = (evt: any) => {
        //   console.log("click");
        //   new Promise((resolve, reject) => {
        //     setTimeout(() => {
        //       console.log('Inside')
        //     }, 300);
        //   }).then(() => {
        //     console.log("resolved")
        //   });
        //   // let handlerCode = el.getAttribute('py-onClick');
        //   // pyodide.runPython(handlerCode);
        // }
    }
}

/** Mount all elements with attribute py-mount into the Python namespace */
export async function mountElements(interpreter: InterpreterClient) {
    const matches: NodeListOf<HTMLElement> = document.querySelectorAll('[py-mount]');
    logger.info(`py-mount: found ${matches.length} elements`);

    if (matches.length > 0) {
        const deprecationMessage =
            'The "py-mount" attribute is deprecated. Please add references to HTML Elements manually in your script.';
        createDeprecationWarning(deprecationMessage, 'py-mount');
    }

    let source = '';
    for (const el of matches) {
        const mountName = el.getAttribute('py-mount') || el.id.split('-').join('_');
        source += `\n${mountName} = Element("${el.id}")`;
    }
    await interpreter.run(source);
}


Review comment (suggestion):
please add a comment to specify in which release the deprecation was added.
It's a bit weird because we don't know yet what the release number will be, maybe we can write something like:
- `deprecated after 2023.03.01`
- `deprecated from version > 2023.03.1`
- `last non-deprecated version: 2023.03.1` 

Code after:
        // el.onclick = (evt: any) => {
        //   console.log("click");
        //   new Promise((resolve, reject) => {
        //     setTimeout(() => {
        //       console.log('Inside')
        //     }, 300);
        //   }).then(() => {
        //     console.log("resolved")
        //   });
        //   // let handlerCode = el.getAttribute('py-onClick');
        //   // pyodide.runPython(handlerCode);
        // }
    }
}

/** Mount all elements with attribute py-mount into the Python namespace */
export async function mountElements(interpreter: InterpreterClient) {
    const matches: NodeListOf<HTMLElement> = document.querySelectorAll('[py-mount]');
    logger.info(`py-mount: found ${matches.length} elements`);

    if (matches.length > 0) {
        //last non-deprecated version: 2023.03.1
        const deprecationMessage =
            'The "py-mount" attribute is deprecated. Please add references to HTML Elements manually in your script.';
        createDeprecationWarning(deprecationMessage, 'py-mount');
    }

    let source = '';
    for (const el of matches) {
        const mountName = el.getAttribute('py-mount') || el.id.split('-').join('_');
        source += `\n${mountName} = Element("${el.id}")`;
    }
    await interpreter.run(source);
}
Repository: pyscript/pyscript
File: pyscriptjs/tests/integration/conftest.py (Python)

Code before:
def dev_server(logger):
    class MyHTTPRequestHandler(SimpleHTTPRequestHandler):
        use_cors = True

        def end_headers(self):
            self.send_my_headers()
            SimpleHTTPRequestHandler.end_headers(self)

        def send_my_headers(self):
            if self.use_cors:
                self.send_header("Cross-Origin-Embedder-Policy", "require-corp")
                self.send_header("Cross-Origin-Opener-Policy", "same-origin")

        def log_message(self, fmt, *args):
            logger.log("http_server", fmt % args, color="blue")

    host, port = "localhost", 8080
    base_url = f"http://{host}:{port}"

    # serve_Run forever under thread
    server = HTTPServer((host, port), MyHTTPRequestHandler)

    thread = threading.Thread(None, server.run)
    thread.start()

    yield server, base_url  # Transition to test here

    # End thread
    server.shutdown()
    thread.join()


Review comment (suggestion):
I know I am the one who suggested it, but it was meant just as a quick hack to move forward, not as the final state.
With the current status we have:
  - a fixture called `dev_sever` which returns a tuple
  - an attribute `PyScriptTest.dev_server` which does *not* contain the fixture, but only the 2nd item

This is very confusing.

A better way to proceed is probably the following:
  - rename the `HTTPServer` class into `DevServer`
  - add an attribute to it called `addr` or `base_url`, which contains what is now in the `base_url` variable
  - then, the `dev_server` fixture can return an instance of `DevServer`, and `PyScriptTest` can just use `dev_server.base_url` to get the address



Code after:

        @classmethod
        def my_headers(cls):
            if cls.enable_cors_headers:
                return {
                    "Cross-Origin-Embedder-Policy": "require-corp",
                    "Cross-Origin-Opener-Policy": "same-origin",
                }
            return {}

        def end_headers(self):
            self.send_my_headers()
            SimpleHTTPRequestHandler.end_headers(self)

        def send_my_headers(self):
            for k, v in self.my_headers().items():
                self.send_header(k, v)

        def log_message(self, fmt, *args):
            logger.log("http_server", fmt % args, color="blue")

    host, port = "localhost", 8080
    base_url = f"http://{host}:{port}"

    # serve_Run forever under thread
    server = DevServer(base_url, (host, port), MyHTTPRequestHandler)

    thread = threading.Thread(None, server.run)
    thread.start()

    yield server  # Transition to test here

    # End thread
    server.shutdown()
    thread.join()
Repository: pyscript/pyscript
File: pyscriptjs/src/python/pyscript/_html.py (Python)

Code before:
            new_id = self.element.id

        clone = self.element.cloneNode(True)
        clone.id = new_id

        if to:
            to.element.appendChild(clone)
            # Inject it into the DOM
            to.element.after(clone)
        else:
            # Inject it into the DOM
            self.element.after(clone)

        return Element(clone.id, clone)

    def remove_class(self, classname):
        classList = self.element.classList
        if isinstance(classname, list):
            classList.remove.apply(classList, classname)
        else:
            classList.remove(classname)

    def add_class(self, classname):
        classList = self.element.classList
        if isinstance(classname, list):
            classList.add.apply(classList, classname)
        else:
            self.element.classList.add(classname)


def add_classes(element, class_list):
    classList = element.classList
    classList.add.apply(classList, class_list.split(" "))


def create(what, id_=None, classes=""):
    element = js.document.createElement(what)
    if id_:
        element.id = id_
    add_classes(element, classes)
    return Element(id_, element)


class PyWidgetTheme:
    def __init__(self, main_style_classes):
        self.main_style_classes = main_style_classes

    def theme_it(self, widget):
        for klass in self.main_style_classes.split(" "):
            widget.classList.add(klass)


Review comment (suggestion):
```suggestion
            classList.add(*classname)
```


Code after:

        clone = self.element.cloneNode(True)
        clone.id = new_id

        if to:
            to.element.appendChild(clone)
            # Inject it into the DOM
            to.element.after(clone)
        else:
            # Inject it into the DOM
            self.element.after(clone)

        return Element(clone.id, clone)

    def remove_class(self, classname):
        classList = self.element.classList
        if isinstance(classname, list):
            classList.remove(*classname)
        else:
            classList.remove(classname)

    def add_class(self, classname):
        classList = self.element.classList
        if isinstance(classname, list):
            classList.add(*classname)
        else:
            self.element.classList.add(classname)


def add_classes(element, class_list):
    classList = element.classList
    classList.add(*class_list.split(" "))


def create(what, id_=None, classes=""):
    element = js.document.createElement(what)
    if id_:
        element.id = id_
    add_classes(element, classes)
    return Element(id_, element)
Repository: pyscript/pyscript
File: pyscriptjs/src/exceptions.ts (TypeScript)

Code before:
        this.name = 'FetchError';
    }
}

export class InstallError extends UserError {
    errorCode: ErrorCode;
    constructor(errorCode: ErrorCode, message: string) {
        super(errorCode, message);
        this.name = 'InstallError';
    }
}

export function _createAlertBanner(
    message: string,
    level: 'error' | 'warning' = 'error',
    messageType: MessageType = 'text',
    logMessage = true,
) {
    switch (`log-${level}-${logMessage}`) {
        case 'log-error-true':
            console.error(message);
            break;
        case 'log-warning-true':
            console.warn(message);
            break;
    }

    const banner = document.createElement('div');
    banner.className = `alert-banner py-${level}`;

    if (messageType === 'html') {
        banner.innerHTML = message;
    } else {
        banner.textContent = message;
    }

    if (level === 'warning') {
        const closeButton = document.createElement('button');

        closeButton.id = 'alert-close-button';
        closeButton.addEventListener('click', () => {
            banner.remove();
        });
        closeButton.innerHTML = CLOSEBUTTON;

        banner.appendChild(closeButton);
    }

    document.body.prepend(banner);
}


Review comment (suggestion):
I haven't seen this logic before, does this just create the object returned from `createElement` with some extra attributes (id, innerHTML?)

This seems like a cool trick!

Code after:
export function _createAlertBanner(
    message: string,
    level: 'error' | 'warning' = 'error',
    messageType: MessageType = 'text',
    logMessage = true,
) {
    switch (`log-${level}-${logMessage}`) {
        case 'log-error-true':
            console.error(message);
            break;
        case 'log-warning-true':
            console.warn(message);
            break;
    }

    const content = messageType === 'html' ? 'innerHTML' : 'textContent';
    const banner = Object.assign(document.createElement('div'), {
        className: `alert-banner py-${level}`,
        [content]: message,
    });

    if (level === 'warning') {
        const closeButton = Object.assign(document.createElement('button'), {
            id: 'alert-close-button',
            innerHTML: CLOSEBUTTON,
        });

        banner.appendChild(closeButton).addEventListener('click', () => {
            banner.remove();
        });
    }

    document.body.prepend(banner);
}
Repository: pyscript/pyscript
File: pyscript.core/esm/worker/class.js (JavaScript)

Code before:
import xworker from "./xworker.js";
import { assign, defineProperty, absoluteURL } from "../utils.js";
import { getText } from "../fetch-utils.js";

/**
 * @typedef {Object} WorkerOptions plugin configuration
 * @prop {string} type the runtime/interpreter type to use
 * @prop {string} [version] the optional runtime version to use
 * @prop {string} [config] the optional config to use within such runtime
 */

export default (...args) =>
    /**
     * A XWorker is a Worker facade able to bootstrap a channel with any desired runtime.
     * @param {string} url the remote file to evaluate on bootstrap
     * @param {WorkerOptions} [options] optional arguments to define the runtime to use
     * @returns {Worker}
     */
    function XWorker(url, options) {
        const worker = xworker();
        const { postMessage } = worker;
        if (args.length) {
            const [type, version] = args;
            options = assign({}, options || { type, version });
            if (!options.type) options.type = type;
        }
        if (options?.config) options.config = absoluteURL(options.config);
        const bootstrap = fetch(url)
            .then(getText)
            .then((code) => postMessage.call(worker, { options, code }));
        return defineProperty(worker, "postMessage", {
            value: (data, ...rest) =>
                bootstrap.then(() => postMessage.call(worker, data, ...rest)),
        });
    };


Review comment (suggestion):
Are these properties intentionally non-writeable non-configurable non-enumerable? If so, why?

Also, a dumb simplification:
```suggestion
                value: (...args) =>
                    bootstrap.then(() =>
                        postMessage.call(worker, ...args),
                    ),
```

Code after:

export default (...args) =>
    /**
     * A XWorker is a Worker facade able to bootstrap a channel with any desired runtime.
     * @param {string} url the remote file to evaluate on bootstrap
     * @param {WorkerOptions} [options] optional arguments to define the runtime to use
     * @returns {Worker}
     */
    function XWorker(url, options) {
        const worker = xworker();
        const { postMessage } = worker;
        if (args.length) {
            const [type, version] = args;
            options = assign({}, options || { type, version });
            if (!options.type) options.type = type;
        }
        if (options?.config) options.config = absoluteURL(options.config);
        const bootstrap = fetch(url)
            .then(getText)
            .then((code) => postMessage.call(worker, { options, code }));
        return defineProperties(worker, {
            postMessage: {
                value: (data, ...rest) =>
                    bootstrap.then(() =>
                        postMessage.call(worker, data, ...rest),
                    ),
            },
            sync: {
                value: coincident(worker),
            },
        });
    };
Repository: pyscript/pyscript
File: pyscript.core/esm/interpreter/micropython.js (JavaScript)

Code before:
    run,
    runAsync,
    runEvent,
    runWorker,
    runWorkerAsync,
    writeFile,
} from "./_python.js";

const type = "micropython";

// REQUIRES INTEGRATION TEST
/* c8 ignore start */
export default {
    type,
    module: (version = "1.20.0-239") =>
        `https://cdn.jsdelivr.net/npm/@micropython/micropython-webassembly-pyscript@${version}/micropython.mjs`,
    async engine({ loadMicroPython }, config, url) {
        const { stderr, stdout, get } = stdio();
        url = url.replace(/\.m?js$/, ".wasm");
        const runtime = await get(loadMicroPython({ stderr, stdout, url }));
        if (config.fetch) await fetchPaths(this, runtime, config.fetch);
        return runtime;
    },
    run,
    runAsync,
    runEvent,
    runWorker,
    runWorkerAsync,
    writeFile,
};
/* c8 ignore stop */


Review comment (suggestion):
why not:
```js
    setGlobal(interpreter, name, value) {
        const id = `__pyscript_${this.type}_${name}`;
        globalThis[id] = value;
        this.run(interpreter, `from js import ${id} as ${name}`);
        delete globalThis[id];
    },
    deleteGlobal(interpreter, name) {
        this.run(interpreter, `del ${name}`);
    },
```

Code after:

const type = "micropython";

// REQUIRES INTEGRATION TEST
/* c8 ignore start */
export default {
    type,
    module: (version = "1.20.0-239") =>
        `https://cdn.jsdelivr.net/npm/@micropython/micropython-webassembly-pyscript@${version}/micropython.mjs`,
    async engine({ loadMicroPython }, config, url) {
        const { stderr, stdout, get } = stdio();
        url = url.replace(/\.m?js$/, ".wasm");
        const runtime = await get(loadMicroPython({ stderr, stdout, url }));
        if (config.fetch) await fetchPaths(this, runtime, config.fetch);
        return runtime;
    },
    setGlobal(interpreter, name, value) {
        const id = `__pyscript_${this.type}_${name}`;
        globalThis[id] = value;
        this.run(interpreter, `from js import ${id};${name}=${id};`);
    },
    deleteGlobal(interpreter, name) {
        const id = `__pyscript_${this.type}_${name}`;
        this.run(interpreter, `del ${id};del ${name}`);
        delete globalThis[id];
    },
    run,
    runAsync,
    writeFile,
};
/* c8 ignore stop */
Repository: pyscript/pyscript
File: pyscript.core/esm/interpreter/_python.js (JavaScript)

Code before:
import { clean, writeFile as writeFileUtil } from "./_utils.js";

// REQUIRES INTEGRATION TEST
/* c8 ignore start */
export const registerJSModule = (interpreter, name, value) => {
    interpreter.registerJsModule(name, value);
};

export const run = (interpreter, code) => interpreter.runPython(clean(code));

export const runAsync = (interpreter, code) =>
    interpreter.runPythonAsync(clean(code));

export const runEvent = async (interpreter, code, event) => {
    // allows method(event) as well as namespace.method(event)
    // it does not allow fancy brackets names for now
    const [name, ...keys] = code.split(".");
    let target = interpreter.globals.get(name);
    let context;
    for (const key of keys) [context, target] = [target, target[key]];
    target.call(context, event);
};

export const writeFile = ({ FS }, path, buffer) =>
    writeFileUtil(FS, path, buffer);
/* c8 ignore stop */


Review comment (suggestion):
You could imagine just using `Promise.resolve()` instead of `await` if you want to ensure that a thenable task is resolved to a Promise. It's not clear which is better.

Code after:
import { clean, writeFile as writeFileUtil } from "./_utils.js";

// REQUIRES INTEGRATION TEST
/* c8 ignore start */
export const registerJSModule = (interpreter, name, value) => {
    interpreter.registerJsModule(name, value);
};

export const run = (interpreter, code) => interpreter.runPython(clean(code));

export const runAsync = (interpreter, code) =>
    interpreter.runPythonAsync(clean(code));

export const runEvent = async (interpreter, code, event) => {
    // allows method(event) as well as namespace.method(event)
    // it does not allow fancy brackets names for now
    const [name, ...keys] = code.split(".");
    let target = interpreter.globals.get(name);
    let context;
    for (const key of keys) [context, target] = [target, target[key]];
    await target.call(context, event);
};

export const writeFile = ({ FS }, path, buffer) =>
    writeFileUtil(FS, path, buffer);
/* c8 ignore stop */
Repository: pyscript/pyscript
File: pyscript.core/src/stdlib/pyscript.py (Python)

Code before:
# export only what we want to expose as `pyscript` module
# but not what is WORKER/MAIN dependent
from _pyscript import window, document, IS_WORKER

# this part is needed to disambiguate between MAIN and WORKER
if IS_WORKER:
    # in workers the display does not have a default ID
    # but there is a sync utility from xworker
    from _pyscript.display import display
    from _pyscript import sync
else:
    # in MAIN both PyWorker and current element target exist
    # so these are both exposed and the display will use,
    # if not specified otherwise, such current element target
    import _pyscript_js
    from _pyscript.display import display as py_display

    PyWorker = _pyscript_js.PyWorker

    def current_target():
        return _pyscript_js.target

    def display(*values, target=None, append=True):
        if target is None:
            target = current_target()

        return py_display(*values, target=target, append=append)


Review comment (suggestion):
Maybe keep this private:
```suggestion
    from _pyscript.display import display as _py_display
```

Code after:
# export only what we want to expose as `pyscript` module
# but not what is WORKER/MAIN dependent
from _pyscript import window, document, IS_WORKER
from _pyscript.display import display as _display

# this part is needed to disambiguate between MAIN and WORKER
if IS_WORKER:
    # in workers the display does not have a default ID
    # but there is a sync utility from xworker
    import polyscript as _polyscript
    from _pyscript import sync

    def current_target():
        return _polyscript.target

else:
    # in MAIN both PyWorker and current element target exist
    # so these are both exposed and the display will use,
    # if not specified otherwise, such current element target
    import _pyscript_js

    PyWorker = _pyscript_js.PyWorker

    def current_target():
        return _pyscript_js.target


# the display provides a handy default target either in MAIN or WORKER
def display(*values, target=None, append=True):
    if target is None:
        target = current_target()

    return _display(*values, target=target, append=append)