ronantakizawa/codereview-bench huggingface dataset:
Repository: pyscript/pyscript
File: pyscriptjs/src/exceptions.ts (TypeScript)

Code before:
import { createAlertBanner} from "./utils"


export class UserError extends Error {
  constructor(message: string, showBanner =false) {
    super(message)
    this.name = "UserError"
    if (showBanner) {
      createAlertBanner(message, "error")
    }
  }
}


Review comment (suggestion):
uhm, this is not what I had in mind.
What I had in mind was something like this (mostly pseudo-code but it should convey the idea):
```typescript
function withUserErrorHandler(fn) {
    try {
        fn();
    }
    catch(err) {
        if (typeof err === "UserError") // I don't know what is the right way to do typechecks 😅
            createAlertBanner(err.message);
        else
            raise;
    }
}

// main.ts
[...]
withUserErrorHandler(globalApp.main.bind(globalApp));
```

In other words:
  - UserErrors should be displayed only when they actually arrive at the top level
  - it is the caller which decides whether the user error needs to be displayed or not, not the raiser
  - it is still possible to catch `UserError` and hide it, if you need
  - if you raise `UserError`, it will NOT be displayed as a JS traceback on the console: this is good because it's not really an "internal error that we need to fix"
  - it is possible to reuse `withUserErrorHandler` in other parts of the code: this is useful e.g. if you want to execute some JS code, ensure that user errors are displayed but *do not* stop the execution if `UserError` is raised (because it will be caught be the try/catch inside `withUserErrorHandler`). Not sure how useful it will be in practice, though.





Code after:
  document.body.prepend(banner)
}

/*
* This function is used to handle UserError, if we see an error of this
* type, we will automatically create a banner on the page that will tell
* the user what went wrong. Note that the error will still stop execution,
* any other errors we will simply throw them and no banner will be shown.
*/
export function withUserErrorHandler(fn) {
  try {
    return fn();
  } catch (error: unknown) {
    if (error instanceof UserError) {
      /*
      *  Display a page-wide error message to show that something has gone wrong with
      *  PyScript or Pyodide during loading. Probably not be used for issues that occur within
      *  Python scripts, since stderr can be routed to somewhere in the DOM
      */
      _createAlertBanner(error.message)
    }
    else {
      throw error
    }
  }
}
Repository: pyscript/pyscript
File: pyscriptjs/src/utils.ts (TypeScript)

Code before:
}

export function globalExport(name: string, obj: object) {
    // attach the given object to the global object, so that it is globally
    // visible everywhere. Should be used very sparingly!

    globalThis[name] = obj;
}

export function getAttribute(el: Element, attr: string): string | null {
    if (el.hasAttribute(attr)) {
        const value = el.getAttribute(attr);
        if (value) {
            return value;
        }
    }
    return null;
}

export function calculatePaths(fetch_cfg: FetchConfig[]) {
    const fetchPaths: string[] = [];
    const paths: string[] = [];
    fetch_cfg.forEach(function (each_fetch_cfg: FetchConfig) {
        const from = each_fetch_cfg.from || "";
        const to_folder = each_fetch_cfg.to_folder || ".";
        const files = each_fetch_cfg.files;
        if (files !== undefined)
        {
            for (const each_f of files)
            {
                const each_fetch_path = [from.endsWith('/') ? from.slice(0, -1) : from, each_f].filter(it => it!== "").join('/');
                fetchPaths.push(each_fetch_path);
                const each_path = [to_folder.endsWith('/') ? to_folder.slice(0, -1) : to_folder, each_f].join('/');
                paths.push(each_path);
            }
        }
        else
        {
            fetchPaths.push(from);
            const filename = from.split('/').pop();
            if (filename === '') {
                throw Error(`Couldn't determine the filename from the path ${from}`);
            }
            else {
                paths.push([to_folder.endsWith('/') ? to_folder.slice(0, -1) : to_folder, filename].join('/'));
            }
        }
    });
    return [paths, fetchPaths];
}


Review comment (suggestion):
this is missing support for `to_file` as described here: https://github.com/pyscript/pyscript/issues/519#issuecomment-1302362956

Code after:
export function globalExport(name: string, obj: object) {
    // attach the given object to the global object, so that it is globally
    // visible everywhere. Should be used very sparingly!

    globalThis[name] = obj;
}

export function getAttribute(el: Element, attr: string): string | null {
    if (el.hasAttribute(attr)) {
        const value = el.getAttribute(attr);
        if (value) {
            return value;
        }
    }
    return null;
}

export function joinPaths(parts: string[], separator = '/') {
    const res = parts.map(function(part) { return part.trim().replace(/(^[/]*|[/]*$)/g, ''); }).filter(p => p!== "").join(separator || '/');
    if (parts[0].startsWith('/'))
    {
        return '/'+res;
    }
    return res;
}
Repository: pyscript/pyscript
File: pyscriptjs/tests/unit/utils.test.ts (TypeScript)

Code before:
    ensureUniqueId(element)

    expect(element.id).toBe("py-internal-0")
  })

  it("ensureUniqueId sets unique id with increasing counter", async () => {
    const secondElement = document.createElement("div")

    expect(element.id).toBe("")
    expect(secondElement.id).toBe("")

    ensureUniqueId(element)
    ensureUniqueId(secondElement)

    // The counter will have been incremented on
    // the previous test, make sure it keeps increasing
    expect(element.id).toBe("py-internal-1")
    expect(secondElement.id).toBe("py-internal-2")
  })
})

describe("JoinPaths", () => {
  it("should remove trailing slashes from the beginning and the end", () => {
    const paths: string[] = ['///abc/d/e///'];
    const joinedPath = joinPaths(paths);
    expect(joinedPath).toStrictEqual('abc/d/e');
  })

  it("should not remove slashes from the middle to preserve protocols such as http", () => {
    const paths: string[] = ['///http://google.com', '///data.txt'];
    const joinedPath = joinPaths(paths);
    expect(joinedPath).toStrictEqual('http://google.com/data.txt');
  })

  it("should not join paths when they are empty strings", () => {
    const paths: string[] = ['', '///hhh/ll/pp///', '', 'kkk'];
    const joinedPath = joinPaths(paths);
    expect(joinedPath).toStrictEqual('hhh/ll/pp/kkk');
  })
})


Review comment (bug):
this looks wrong.
If I do `jointPaths("/tmp", "foo.txt")` I expect `/tmp/foo.txt`, not `tmp/foo.txt`

Code after:
    ensureUniqueId(element)

    expect(element.id).toBe("py-internal-0")
  })

  it("ensureUniqueId sets unique id with increasing counter", async () => {
    const secondElement = document.createElement("div")

    expect(element.id).toBe("")
    expect(secondElement.id).toBe("")

    ensureUniqueId(element)
    ensureUniqueId(secondElement)

    // The counter will have been incremented on
    // the previous test, make sure it keeps increasing
    expect(element.id).toBe("py-internal-1")
    expect(secondElement.id).toBe("py-internal-2")
  })
})

describe("JoinPaths", () => {
  it("should remove trailing slashes from the beginning and the end", () => {
    const paths: string[] = ['///abc/d/e///'];
    const joinedPath = joinPaths(paths);
    expect(joinedPath).toStrictEqual('/abc/d/e');
  })

  it("should not remove slashes from the middle to preserve protocols such as http", () => {
    const paths: string[] = ['http://google.com', '///data.txt'];
    const joinedPath = joinPaths(paths);
    expect(joinedPath).toStrictEqual('http://google.com/data.txt');
  })

  it("should not join paths when they are empty strings", () => {
    const paths: string[] = ['', '///hhh/ll/pp///', '', 'kkk'];
    const joinedPath = joinPaths(paths);
    expect(joinedPath).toStrictEqual('hhh/ll/pp/kkk');
  })
})
Repository: pyscript/pyscript
File: pyscriptjs/src/plugins/fetch.ts (TypeScript)

Code before:
        const files = each_fetch_cfg.files;
        if (files !== undefined)
        {
            if (to_file !== undefined)
            {
                throw new UserError(
                    UserError.ErrorCode.FETCH_PARAMETER_ERROR,
                    `Cannot use 'to_file' and 'files' parameters together!`
                );
            }
            for (const each_f of files)
            {
                const each_fetch_path = joinPaths([from, each_f]);
                fetchPaths.push(each_fetch_path);
                const each_path = joinPaths([to_folder, each_f]);
                paths.push(each_path);
            }
        }
        else
        {
            fetchPaths.push(from);
            const filename = to_file || from.split('/').pop();
            if (filename === '') {
                throw new UserError(
                    UserError.ErrorCode.FETCH_PARAMETER_ERROR,
                    `Couldn't determine the filename from the path ${from}, supply ${to_file} parameter!`
                );
            }
            else {
                paths.push(joinPaths([to_folder, filename]));
            }
        }
    });
    return [paths, fetchPaths];
}


Review comment (suggestion):
I guess this is carried from a previous mistake but `${to_file} parameter` should be `"to_file" parameter` since we don't want to use string interpolation here i.e. we don't want to use the value of `to_file` inside the message, but instead communicate that you need to use this.

Code after:
        const files = each_fetch_cfg.files;
        if (files !== undefined)
        {
            if (to_file !== undefined)
            {
                throw new UserError(
                    ErrorCode.BAD_CONFIG,
                    `Cannot use 'to_file' and 'files' parameters together!`
                );
            }
            for (const each_f of files)
            {
                const each_fetch_path = joinPaths([from, each_f]);
                fetchPaths.push(each_fetch_path);
                const each_path = joinPaths([to_folder, each_f]);
                paths.push(each_path);
            }
        }
        else
        {
            fetchPaths.push(from);
            const filename = to_file || from.split('/').pop();
            if (filename === '') {
                throw new UserError(
                    ErrorCode.BAD_CONFIG,
                    `Couldn't determine the filename from the path ${from}, please supply 'to_file' parameter.`
                );
            }
            else {
                paths.push(joinPaths([to_folder, filename]));
            }
        }
    });
    return [paths, fetchPaths];
}
Repository: pyscript/pyscript
File: pyscriptjs/src/utils.ts (TypeScript)

Code before:
    }
    return null;
}

export function joinPaths(parts: string[], separator = '/') {
    const res = parts.map(function(part) { return part.trim().replace(/(^[/]*|[/]*$)/g, ''); }).filter(p => p!== "").join(separator || '/');
    if (parts[0].startsWith('/'))
    {
        return '/'+res;
    }
    return res;
}

export function createDeprecationWarning(msg: string, elementName: string): void {
    const banners = document.getElementsByClassName('alert-banner py-warning');
    let bannerCount = 0;
    for (const banner of banners) {
        if (banner.innerHTML.includes(elementName)) {
            bannerCount++;
        }
    }
    if (bannerCount == 0) {
        _createAlertBanner(msg, "warning");
    }
}

export async function fetchIt(url: string, options?: RequestInit): Promise<Response> {
    const response = await fetch(url, options);
    if (response.status !== 200) {
        const errorMsg = `Fetching from URL ${url} failed with error ${response.status} (${response.statusText}).`;
        switch(response.status) {
            case 404:
                throw new FetchError(
                    FetchError.ErrorCode.FETCH_NOT_FOUND_ERROR,
                    errorMsg
                );
            case 401:
                throw new FetchError(
                    FetchError.ErrorCode.FETCH_UNAUTHORIZED_ERROR,
                    errorMsg
                );
            case 403:
                throw new FetchError(
                    FetchError.ErrorCode.FETCH_FORBIDDEN_ERROR,
                    errorMsg
                );
            case 500:
                throw new FetchError(
                    FetchError.ErrorCode.FETCH_SERVER_ERROR,
                    errorMsg
                );

Review comment (suggestion):
this definitely lacks a docstring :).
I love the idea of having a fetch wrapper which does "the right thing".

Also, [follow-up]
I suggest to move it on its own file though: basically, I don't want `utils.ts` to become "the place where we put random unrelated stuff.
I wonder whether we can find a better name than `fetchIt`. Maybe `robustFetch` or `fetchRobust`? But let's discuss it elsewhere

Code after:
    }
    return null;
}

export function joinPaths(parts: string[], separator = '/') {
    const res = parts.map(function(part) { return part.trim().replace(/(^[/]*|[/]*$)/g, ''); }).filter(p => p!== "").join(separator || '/');
    if (parts[0].startsWith('/'))
    {
        return '/'+res;
    }
    return res;
}

export function createDeprecationWarning(msg: string, elementName: string): void {
    const banners = document.getElementsByClassName('alert-banner py-warning');
    let bannerCount = 0;
    for (const banner of banners) {
        if (banner.innerHTML.includes(elementName)) {
            bannerCount++;
        }
    }
    if (bannerCount == 0) {
        _createAlertBanner(msg, "warning");
    }
}
Repository: pyscript/pyscript
File: pyscriptjs/src/main.ts (TypeScript)

Code before:
import './styles/pyscript_base.css';

import { loadConfigFromElement } from './pyconfig';
import type { AppConfig } from './pyconfig';
import type { Runtime } from './runtime';
import { PluginManager } from './plugin';
import { make_PyScript, initHandlers, mountElements } from './components/pyscript';
import { PyodideRuntime } from './pyodide';
import { getLogger } from './logger';
import { handleFetchError, showWarning, globalExport } from './utils';
import { calculatePaths } from './plugins/fetch';
import { createCustomElements } from './components/elements';
import { UserError, _createAlertBanner } from "./exceptions"
import { type Stdio, StdioMultiplexer, DEFAULT_STDIO } from './stdio';
import { PyTerminalPlugin } from './plugins/pyterminal';
import { SplashscreenPlugin } from './plugins/splashscreen';
import { ImportmapPlugin } from './plugins/importmap';

const logger = getLogger('pyscript/main');

/* High-level overview of the lifecycle of a PyScript App:

   1. pyscript.js is loaded by the browser. PyScriptApp().main() is called

   2. loadConfig(): search for py-config and compute the config for the app

   3. (it used to be "show the splashscreen", but now it's a plugin)

   4. loadRuntime(): start downloading the actual runtime (e.g. pyodide.js)

   --- wait until (4) has finished ---

Review comment (suggestion):
I think that this const should not live in `runtime.ts` FWIW, it doesn't make sense.
Probably the best place is here, directly in `main.ts`

Code after:
import './styles/pyscript_base.css';

import { loadConfigFromElement } from './pyconfig';
import type { AppConfig } from './pyconfig';
import type { Runtime } from './runtime';
import { version } from './runtime';
import { PluginManager } from './plugin';
import { make_PyScript, initHandlers, mountElements } from './components/pyscript';
import { PyodideRuntime } from './pyodide';
import { getLogger } from './logger';
import { handleFetchError, showWarning, globalExport } from './utils';
import { calculatePaths } from './plugins/fetch';
import { createCustomElements } from './components/elements';
import { UserError, _createAlertBanner } from "./exceptions"
import { type Stdio, StdioMultiplexer, DEFAULT_STDIO } from './stdio';
import { PyTerminalPlugin } from './plugins/pyterminal';
import { SplashscreenPlugin } from './plugins/splashscreen';
import { ImportmapPlugin } from './plugins/importmap';

const logger = getLogger('pyscript/main');

/* High-level overview of the lifecycle of a PyScript App:

   1. pyscript.js is loaded by the browser. PyScriptApp().main() is called

   2. loadConfig(): search for py-config and compute the config for the app

   3. (it used to be "show the splashscreen", but now it's a plugin)

   4. loadRuntime(): start downloading the actual runtime (e.g. pyodide.js)
Repository: pyscript/pyscript
File: pyscriptjs/src/plugins/python/py_markdown.py (Python)

Code before:
import html
from textwrap import dedent

from js import console
from markdown import markdown
from pyscript import Plugin, console

console.warn(
    "WARNING: This plugin is still in a very experimental phase and will likely change"
    " and potentially break in the future releases. Use it with caution."
)


class MyPlugin(Plugin):
    def configure(self, config):
        console.log(f"configuration received: {config}")

    def afterStartup(self, runtime):
        console.log(f"runtime received: {runtime}")


plugin = MyPlugin("py-markdown")


@plugin.register_custom_element("py-md")
class PyMarkdown:
    def __init__(self, element):
        self.element = element

    def connect(self):
        unescaped_content = html.unescape(self.element.originalInnerHTML)

Review comment (suggestion):
Since we are importing console on line 4, should we remove it from here?

```suggestion
from pyscript import Plugin
```

Code after:
import html
from textwrap import dedent

from js import console
from markdown import markdown
from pyscript import Plugin

console.warn(
    "WARNING: This plugin is still in a very experimental phase and will likely change"
    " and potentially break in the future releases. Use it with caution."
)


class MyPlugin(Plugin):
    def configure(self, config):
        console.log(f"configuration received: {config}")

    def afterStartup(self, runtime):
        console.log(f"runtime received: {runtime}")


plugin = MyPlugin("py-markdown")


@plugin.register_custom_element("py-md")
class PyMarkdown:
    def __init__(self, element):
        self.element = element

    def connect(self):
        unescaped_content = html.unescape(self.element.originalInnerHTML)
Repository: pyscript/pyscript
File: pyscriptjs/src/interpreter_client.ts (TypeScript)

Code before:
import type { AppConfig } from './pyconfig';
import { RemoteInterpreter } from './remote_interpreter';
import type {  PyProxy } from 'pyodide';
import { getLogger } from './logger';
import type { Stdio } from './stdio';

const logger = getLogger('pyscript/interpreter');

export class InterpreterClient extends Object {

    _remote: RemoteInterpreter;
    config: AppConfig;
    globals: PyProxy;
    stdio: Stdio;

    constructor(config: AppConfig, stdio: Stdio) {
        super();
        this.config = config;
        this._remote = new RemoteInterpreter(this.config.interpreters[0].src);
        this.stdio = stdio;
    }

    async initializeRemote(): Promise<void> {
        await this._remote.loadInterpreter(this.config, this.stdio);
        this.globals = this._remote.globals;
    }

    async run(code: string): Promise<{result: any}> {
        return await this._remote.run(code);
    }

    async runButDontRaise(code: string): Promise<unknown> {
        let result: unknown;
        try {
            result = (await this.run(code)).result;
        } catch (error: unknown) {
            logger.error('Error:', error);
        }
        return result;
    }

Review comment (suggestion):
I think this should perhaps be called something else, perhaps `type` so we can read this as:

`interpreter.type` is `Pyodide type` or `Micropython type`

Code after:
import type { AppConfig } from './pyconfig';
import { RemoteInterpreter } from './remote_interpreter';
import type { PyProxy } from 'pyodide';
import { getLogger } from './logger';
import type { Stdio } from './stdio';

const logger = getLogger('pyscript/interpreter');

/*
InterpreterClient class is responsible to request code execution
(among other things) from a `RemoteInterpreter`
*/
export class InterpreterClient extends Object {

    _remote: RemoteInterpreter;
    config: AppConfig;
    /**
     * global symbols table for the underlying interface.
     * */
    globals: PyProxy;
    stdio: Stdio;

    constructor(config: AppConfig, stdio: Stdio) {
        super();
        this.config = config;
        this._remote = new RemoteInterpreter(this.config.interpreters[0].src);
        this.stdio = stdio;
    }

    /**
     * initializes the remote interpreter, which further loads the underlying
     * interface.
     * */
    async initializeRemote(): Promise<void> {
        await this._remote.loadInterpreter(this.config, this.stdio);
        this.globals = this._remote.globals;
    }

    /**
     * delegates the code to be run to the underlying interface of
Repository: pyscript/pyscript
File: pyscriptjs/src/plugins/pyterminal.ts (TypeScript)

Code before:
import type { PyScriptApp } from '../main';
import type { AppConfig } from '../pyconfig';
import { Plugin } from '../plugin';
import { UserError, ErrorCode } from '../exceptions';
import { getLogger } from '../logger';
import { type Stdio } from '../stdio';
import { InterpreterClient } from '../interpreter_client';

type AppConfigStyle = AppConfig & { terminal?: boolean | 'auto'; docked?: boolean | 'docked' };

const logger = getLogger('py-terminal');

const validate = (name: string, dflt: string, value?: boolean | string) => {
    if (value !== undefined && value !== true && value !== false && value !== dflt) {
        const got = JSON.stringify(value);
        throw new UserError(
            ErrorCode.BAD_CONFIG,
            `Invalid value for config.${name}: the only accepted` +
                `values are true, false and "${dflt}", got "${got}".`,
        );
    }
};

export class PyTerminalPlugin extends Plugin {
    app: PyScriptApp;

    constructor(app: PyScriptApp) {
        super();
        this.app = app;
    }

    configure(config: AppConfigStyle) {
        // validate the terminal config and handle default values
        const { terminal: t, docked: d } = config;

Review comment (suggestion):
I see we were passing the terminal config directly into the `configure`, do you think we should add these to the `AppConfig` interface? 🤔 

Code after:
import type { PyScriptApp } from '../main';
import type { AppConfig } from '../pyconfig';
import { Plugin } from '../plugin';
import { UserError, ErrorCode } from '../exceptions';
import { getLogger } from '../logger';
import { type Stdio } from '../stdio';
import { InterpreterClient } from '../interpreter_client';

type AppConfigStyle = AppConfig & { terminal?: boolean | 'auto'; docked?: boolean | 'docked' };

const logger = getLogger('py-terminal');

const validate = (config: AppConfigStyle, name: string, default_: string) => {
    const value = config[name] as undefined | boolean | string;
    if (value !== undefined && value !== true && value !== false && value !== default_) {
        const got = JSON.stringify(value);
        throw new UserError(
            ErrorCode.BAD_CONFIG,
            `Invalid value for config.${name}: the only accepted` +
                `values are true, false and "${default_}", got "${got}".`,
        );
    }
    if (value === undefined) {
        config[name] = default_;
    }
};

export class PyTerminalPlugin extends Plugin {
    app: PyScriptApp;

    constructor(app: PyScriptApp) {
        super();
        this.app = app;
    }