ronantakizawa/codereview-bench huggingface dataset:
Repository: ianstormtaylor/slate
File: packages/slate-dom/src/plugin/with-dom.ts (TypeScript)

Code before:
          matches.push(...getMatches(e, Path.parent(op.newPath)))
          changedPath = op.path
        }

        const changedNode = Node.get(editor, Path.parent(changedPath))
        const changedNodeKey = DOMEditor.findKey(e, changedNode)
        const changedPathRef = Editor.pathRef(e, Path.parent(changedPath))
        pathRefMatches.push([changedPathRef, changedNodeKey])

        break
      }
    }

    apply(op)

    switch (op.type) {
      case 'insert_node':
      case 'remove_node':
      case 'merge_node':
      case 'move_node':
      case 'split_node':
      case 'insert_text':
      case 'remove_text':
      case 'set_selection': {
        // FIXME: Rename to something like IS_DOM_EDITOR_DESYNCED
        // to better reflect reality, see #5792 
        IS_NODE_MAP_DIRTY.set(e, true)
      }
    }

    for (const [path, key] of matches) {
      const [node] = Editor.node(e, path)
      NODE_TO_KEY.set(node, key)
    }

    for (const [pathRef, key] of pathRefMatches) {
      if (pathRef.current) {
        const [node] = Editor.node(e, pathRef.current)
        NODE_TO_KEY.set(node, key)
      }

      pathRef.unref()
    }
  }

  e.setFragmentData = (data: Pick<DataTransfer, 'getData' | 'setData'>) => {
    const { selection } = e

    if (!selection) {
      return
    }

Review comment (suggestion):
```suggestion
        // to better reflect reality, see #5792
```

Code after:
          matches.push(...getMatches(e, Path.parent(op.newPath)))
          changedPath = op.path
        }

        const changedNode = Node.get(editor, Path.parent(changedPath))
        const changedNodeKey = DOMEditor.findKey(e, changedNode)
        const changedPathRef = Editor.pathRef(e, Path.parent(changedPath))
        pathRefMatches.push([changedPathRef, changedNodeKey])

        break
      }
    }

    apply(op)

    switch (op.type) {
      case 'insert_node':
      case 'remove_node':
      case 'merge_node':
      case 'move_node':
      case 'split_node':
      case 'insert_text':
      case 'remove_text':
      case 'set_selection': {
        // FIXME: Rename to something like IS_DOM_EDITOR_DESYNCED
        // to better reflect reality, see #5792
        IS_NODE_MAP_DIRTY.set(e, true)
      }
    }

    for (const [path, key] of matches) {
      const [node] = Editor.node(e, path)
      NODE_TO_KEY.set(node, key)
    }

    for (const [pathRef, key] of pathRefMatches) {
      if (pathRef.current) {
        const [node] = Editor.node(e, pathRef.current)
        NODE_TO_KEY.set(node, key)
      }

      pathRef.unref()
    }
  }

  e.setFragmentData = (data: Pick<DataTransfer, 'getData' | 'setData'>) => {
    const { selection } = e

    if (!selection) {
      return
    }
Repository: ianstormtaylor/slate
File: packages/slate/src/interfaces/element.ts (TypeScript)

Code before:
    elementVal: string,
    elementKey?: string
  ) => value is T

  /**
   * Check if an element matches set of properties.
   *
   * Note: this checks custom properties, and it does not ensure that any
   * children are equivalent.
   */
  matches: (element: Element, props: Partial<Element>) => boolean
}

/**
 * Shared the function with isElementType utility
 */
const isElement = (
  value: any,
  { deep = false }: ElementIsElementOptions = {}
): value is Element => {
  if (!value || typeof value !== 'object') return false

  // PERF: No need to use the full Editor.isEditor here
  const isEditor = typeof value.apply === 'function'
  if (isEditor) return false

  const isChildrenValid = deep
    ? Node.isNodeList(value.children)
    : Array.isArray(value.children)

  return isChildrenValid
}

// eslint-disable-next-line no-redeclare
export const Element: ElementInterface = {
  isAncestor(
    value: any,
    { deep = false }: ElementIsElementOptions = {}
  ): value is Ancestor {
    return (
      !!value &&
      typeof value === 'object' &&
      Node.isNodeList(value.children, { deep })
    )
  },

  isElement,

  isElementList(
    value: any,
    { deep = false }: ElementIsElementOptions = {}

Review comment (suggestion):
I suppose it might be out of scope for this PR, but it would be nice if we could use `unknown` instead of `any` here.

Code after:
  isElementProps: (props: any) => props is Partial<Element>

  /**
   * Check if a value implements the `Element` interface and has elementKey with selected value.
   * Default it check to `type` key value
   */
  isElementType: <T extends Element>(
    value: any,
    elementVal: string,
    elementKey?: string
  ) => value is T

  /**
   * Check if an element matches set of properties.
   *
   * Note: this checks custom properties, and it does not ensure that any
   * children are equivalent.
   */
  matches: (element: Element, props: Partial<Element>) => boolean
}

/**
 * Shared the function with isElementType utility
 */
const isElement = (
  value: any,
  { deep = false }: ElementIsElementOptions = {}
): value is Element => {
  if (!isObject(value)) return false

  // PERF: No need to use the full Editor.isEditor here
  const isEditor = typeof value.apply === 'function'
  if (isEditor) return false

  const isChildrenValid = deep
    ? Node.isNodeList(value.children)
    : Array.isArray(value.children)

  return isChildrenValid
}

// eslint-disable-next-line no-redeclare
export const Element: ElementInterface = {
  isAncestor(
    value: any,
    { deep = false }: ElementIsElementOptions = {}
  ): value is Ancestor {
    return isObject(value) && Node.isNodeList(value.children, { deep })
  },

  isElement,
Repository: ianstormtaylor/slate
File: packages/slate/src/editor/above.ts (TypeScript)

Code before:
import { Editor, EditorInterface } from '../interfaces/editor'
import { Range } from '../interfaces'
import { Path } from '../interfaces/path'

export const above: EditorInterface['above'] = (editor, options = {}) => {
  const {
    voids = false,
    mode = 'lowest',
    at = editor.selection,
    match,
  } = options

  if (!at) {
    return
  }

  let path = Editor.path(editor, at)

  // start with the direct ancestor -- unless its already the common ancestor of a cross-node range
  if (!Range.isRange(at) || Path.equals(at.focus.path, at.anchor.path)) {
    if (path.length === 0) return
    path = Path.parent(path)
  }

  const reverse = mode === 'lowest'

  for (const entry of Editor.levels(editor, {
    at: path,
    voids,
    match,
    reverse,
  })) {
    return entry
  }
}


Review comment (suggestion):
I had to dig into `Editor.path` to figure out what was going on here. Perhaps if you add a comment like:

```ts
  // If `at` is a range that includes multiple paths, `Editor.path` already
  // returns the common ancestor of those paths. Otherwise, start with the
  // direct ancestor.
```

Code after:
import { Editor, EditorInterface } from '../interfaces/editor'
import { Range } from '../interfaces'
import { Path } from '../interfaces/path'

export const above: EditorInterface['above'] = (editor, options = {}) => {
  const {
    voids = false,
    mode = 'lowest',
    at = editor.selection,
    match,
  } = options

  if (!at) {
    return
  }

  let path = Editor.path(editor, at)

  // If `at` is a Range that spans mulitple nodes, `path` will be their common ancestor.
  // Otherwise `path` will be a text node and/or the same as `at`, in which cases we want to start with its parent.
  if (!Range.isRange(at) || Path.equals(at.focus.path, at.anchor.path)) {
    if (path.length === 0) return
    path = Path.parent(path)
  }

  const reverse = mode === 'lowest'

  const [firstMatch] = Editor.levels(editor, {
    at: path,
    voids,
    match,
    reverse,
  })
  return firstMatch // if nothing matches this returns undefined
}
Repository: ianstormtaylor/slate
File: packages/slate/src/transforms-selection/move.ts (TypeScript)

Code before:
import { SelectionTransforms } from '../interfaces/transforms/selection'
import { Range } from '../interfaces/range'
import { Editor } from '../interfaces/editor'
import { Transforms } from '../interfaces/transforms'

export const move: SelectionTransforms['move'] = (editor, options = {}) => {
  const { selection } = editor
  const { distance = 1, unit = 'character', reverse = false } = options
  let { edge = null } = options

  if (!selection) {
    return
  }

  if (edge === 'start') {
    edge = Range.isBackward(selection) ? 'focus' : 'anchor'
  }

  if (edge === 'end') {
    edge = Range.isBackward(selection) ? 'anchor' : 'focus'
  }

  const { anchor, focus } = selection
  const opts = { distance, unit, ignoreNonSelectable: false }
  const props: Partial<Range> = {}

  if (edge == null || edge === 'anchor') {
    const point = reverse
      ? Editor.before(editor, anchor, opts)
      : Editor.after(editor, anchor, opts)

    if (point) {
      props.anchor = point
    }
  }

  if (edge == null || edge === 'focus') {
    const point = reverse
      ? Editor.before(editor, focus, opts)
      : Editor.after(editor, focus, opts)

    if (point) {
      props.focus = point
    }
  }

  Transforms.setSelection(editor, props)
}


Review comment (suggestion):
`false` is already the default value of `ignoreNonSelectable`.

- `ignoreNonSelectable: false` means **do not** ignore positions inside non-selectable elements (i.e. **do not** skip over them)
- `ignoreNonSelectable: true` means **do** ignore positions inside non-selectable elements (i.e. **do** skip over them)

Perhaps `skipNonSelectable` or `selectableOnly` would have been a less ambiguous name; sorry about that.

Code after:
import { SelectionTransforms } from '../interfaces/transforms/selection'
import { Range } from '../interfaces/range'
import { Editor } from '../interfaces/editor'
import { Transforms } from '../interfaces/transforms'

export const move: SelectionTransforms['move'] = (editor, options = {}) => {
  const { selection } = editor
  const { distance = 1, unit = 'character', reverse = false } = options
  let { edge = null } = options

  if (!selection) {
    return
  }

  if (edge === 'start') {
    edge = Range.isBackward(selection) ? 'focus' : 'anchor'
  }

  if (edge === 'end') {
    edge = Range.isBackward(selection) ? 'anchor' : 'focus'
  }

  const { anchor, focus } = selection
  const opts = { distance, unit }
  const props: Partial<Range> = {}

  if (edge == null || edge === 'anchor') {
    const point = reverse
      ? Editor.before(editor, anchor, opts)
      : Editor.after(editor, anchor, opts)

    if (point) {
      props.anchor = point
    }
  }

  if (edge == null || edge === 'focus') {
    const point = reverse
      ? Editor.before(editor, focus, opts)
      : Editor.after(editor, focus, opts)

    if (point) {
      props.focus = point
    }
  }

  Transforms.setSelection(editor, props)
}
Repository: ianstormtaylor/slate
File: site/examples/ts/components/index.tsx (TypeScript)

Code before:

interface BaseProps {
  className: string
  [key: string]: unknown
}

export const Button = React.forwardRef(
  (
    {
      className,
      active,
      reversed,
      ...props
    }: PropsWithChildren<
      {
        active: boolean
        reversed: boolean
      } & BaseProps
    >,
    ref: Ref<HTMLSpanElement>
  ) => (
    <span
      {...props}
      ref={ref}
      tabIndex={0}
      role="button"
      className={cx(
        className,
        css`
          cursor: pointer;
          color: ${reversed
            ? active
              ? 'white'
              : '#aaa'
            : active
            ? 'black'
            : '#ccc'};
        `
      )}
    />
  )
)

export const Icon = React.forwardRef(
  (
    { className, ...props }: PropsWithChildren<BaseProps>,
    ref: Ref<HTMLSpanElement>
  ) => (
    <span
      {...props}
      ref={ref}

Review comment (suggestion):
Adding these will make the button focusable and tell assistive technology that the element is a button, but since it's a `<span>`, keyboard users will be unable to activate it using the enter or space keys.

In addition, most places where this button is used (editor toolbar buttons) are using the unfortunate pattern of:
```ts
onMouseDown={event => {
  event.preventDefault()
  actualClickHandler()
}}
```
This means that even if the `<span>` were switched to a button, keyboard users would be unable to activate it. 

There's actually a good reason for preventing the mousedown event, since it prevents mouse users' focus from leaving the editor when they click the button. This has to happen on mousedown/pointerdown since it's too late to prevent focus loss by the time the click event is fired. However, it would be much better to use:
```ts
onPointerDown={event => event.preventDefault()}
onClick={actualClickHandler}
```
This achieves the same thing without breaking keyboard accessibility.

Code after:

interface BaseProps {
  className: string
  [key: string]: unknown
}

export const Button = React.forwardRef(
  (
    {
      className,
      active,
      reversed,
      ...props
    }: PropsWithChildren<
      {
        active: boolean
        reversed: boolean
      } & BaseProps
    >,
    ref: Ref<HTMLButtonElement>
  ) => (
    <button
      {...props}
      ref={ref}
      className={cx(
        css`
          border: none;
          background: none;
          padding: 0;
          cursor: pointer;
          color: ${reversed
            ? active
              ? 'white'
              : '#aaa'
            : active
            ? 'black'
            : '#ccc'};
        `,
        className
      )}
    />
  )
)

export const Icon = React.forwardRef(
  (
    { className, ...props }: PropsWithChildren<BaseProps>,
    ref: Ref<HTMLSpanElement>
  ) => (
    <span
      {...props}
Repository: ianstormtaylor/slate
File: site/pages/examples/[example].tsx (TypeScript)

Code before:
  />
)

const TabListUnderlay = ({
  isVisible,
  ...props
}: React.HTMLAttributes<HTMLDivElement> & { isVisible?: boolean }) => (
  <div
    {...props}
    className={css`
      background-color: rgba(200, 200, 200, 0.8);
      display: ${isVisible ? 'block' : 'none'};
      height: 100%;
      top: 0;
      position: fixed;
      width: 100%;
      z-index: 2;
    `}
  />
)

const TabButton = (props: React.HTMLAttributes<HTMLSpanElement>) => (
  <button
    {...props}
    aria-label="Toggle examples menu"
    aria-expanded={props['aria-expanded']}
    aria-haspopup="menu"
    className={css`
      margin-left: 0.8em;
      background: none;
      border: none;
      cursor: pointer;
      padding: 0;

      &:hover {
        cursor: pointer;
      }

      .material-icons {
        color: #aaa;
        font-size: 24px;
      }
    `}
  />
)

const Tab = React.forwardRef(
  (
    {
      active,
      href,

Review comment (suggestion):
This is already covered by `{...props}`.

```suggestion
```

Code after:
      visibility: ${isVisible ? 'visible' : 'hidden'};
    `}
  />
)

const TabListUnderlay = ({
  isVisible,
  ...props
}: React.HTMLAttributes<HTMLDivElement> & { isVisible?: boolean }) => (
  <div
    {...props}
    className={css`
      background-color: rgba(200, 200, 200, 0.8);
      display: ${isVisible ? 'block' : 'none'};
      height: 100%;
      top: 0;
      position: fixed;
      width: 100%;
      z-index: 2;
    `}
  />
)

const TabButton = (props: React.HTMLAttributes<HTMLSpanElement>) => (
  <button
    {...props}
    aria-label="Toggle examples menu"
    aria-haspopup="menu"
    className={css`
      margin-left: 0.8em;
      background: none;
      border: none;
      cursor: pointer;
      padding: 0;

      &:hover {
        cursor: pointer;
      }

      .material-icons {
        color: #aaa;
        font-size: 24px;
      }
    `}
  />
)

const Tab = React.forwardRef(
  (
    {
      active,
Repository: imartinez/privateGPT
File: privateGPT.py (Python)

Code before:
        print(query)
        print("\n> Answer:")
        print(answer)

        # Print the relevant sources used for the answer, if source is True
        if not hide_source and docs:
            for document in docs:
                print("\n> " + document.metadata["source"] + ":")
                print(document.page_content)

def parse_arguments():
    parser = argparse.ArgumentParser()
    parser.add_argument("--hide-source", "-S", action='store_true',
                        help='Use this flag to disable printing of source documents used for answers.')

    parser.add_argument("--mute-stream", "-M",
                        action='store_true',
                        help='Use this flag to disable the streaming StdOut callback for LLMs.')

    return parser.parse_args()


if __name__ == "__main__":
    # Parse the command line arguments
    args = parse_arguments()
    main(hide_source=args.hide_source, mute_stream=args.mute_stream)


Review comment (suggestion):
This isn't great IMHO. I don't see any reason to pass the `args` into `main`, just call `parse_arguments()` inside `main` directly. As is, every time we add a new CLI argument we have to add a new parameter.

Code after:
        # Print the result
        print("\n\n> Question:")
        print(query)
        print("\n> Answer:")
        print(answer)

        # Print the relevant sources used for the answer
        for document in docs:
            print("\n> " + document.metadata["source"] + ":")
            print(document.page_content)

def parse_arguments():
    parser = argparse.ArgumentParser(description='privateGPT: Ask questions to your documents without an internet connection, '
                                                 'using the power of LLMs.')
    parser.add_argument("--hide-source", "-S", action='store_true',
                        help='Use this flag to disable printing of source documents used for answers.')

    parser.add_argument("--mute-stream", "-M",
                        action='store_true',
                        help='Use this flag to disable the streaming StdOut callback for LLMs.')

    return parser.parse_args()


if __name__ == "__main__":
    main()
Repository: imartinez/privateGPT
File: private_gpt/__init__.py (Python)

Code before:
"""private-gpt."""
import logging
import os

# Set to 'DEBUG' to have extensive logging turned on, even for libraries
ROOT_LOG_LEVEL = "DEBUG"

PRETTY_LOG_FORMAT = (
    "%(asctime)s.%(msecs)03d [%(levelname)-8s] %(name)+25s - %(message)s"
)
logging.basicConfig(level=ROOT_LOG_LEVEL, format=PRETTY_LOG_FORMAT, datefmt="%H:%M:%S")
logging.captureWarnings(True)

# Disable gradio analytics
# This is done this way because gradio does not solely rely on what values are
# passed to gr.Blocks(enable_analytics=...) but also on the environment
# variable GRADIO_ANALYTICS_ENABLED. `gradio.strings` actually reads this env
# directly, so to fully disable gradio analytics we need to set this env var.
os.environ["GRADIO_ANALYTICS_ENABLED"] = "False"

# Disable chromaDB telemetry
# It is already disabled, see PR#1144
# os.environ["ANONYMIZED_TELEMETRY"] = "False"


Review comment (suggestion):
Oooops - this change is not intended
```suggestion
ROOT_LOG_LEVEL = "INFO"
```

Code after:
"""private-gpt."""
import logging
import os

# Set to 'DEBUG' to have extensive logging turned on, even for libraries
ROOT_LOG_LEVEL = "INFO"

PRETTY_LOG_FORMAT = (
    "%(asctime)s.%(msecs)03d [%(levelname)-8s] %(name)+25s - %(message)s"
)
logging.basicConfig(level=ROOT_LOG_LEVEL, format=PRETTY_LOG_FORMAT, datefmt="%H:%M:%S")
logging.captureWarnings(True)

# Disable gradio analytics
# This is done this way because gradio does not solely rely on what values are
# passed to gr.Blocks(enable_analytics=...) but also on the environment
# variable GRADIO_ANALYTICS_ENABLED. `gradio.strings` actually reads this env
# directly, so to fully disable gradio analytics we need to set this env var.
os.environ["GRADIO_ANALYTICS_ENABLED"] = "False"

# Disable chromaDB telemetry
# It is already disabled, see PR#1144
# os.environ["ANONYMIZED_TELEMETRY"] = "False"
Repository: imartinez/privateGPT
File: private_gpt/server/ingest/ingest_service.py (Python)

Code before:
import logging
import tempfile
from pathlib import Path
from typing import TYPE_CHECKING, Any, AnyStr

from injector import inject, singleton
from llama_index import (
    Document,
    JSONReader,
    ServiceContext,
    StorageContext,
    StringIterableReader,
    VectorStoreIndex,
    load_index_from_storage,
)
from llama_index.node_parser import SentenceWindowNodeParser
from llama_index.readers.file.base import (
    DEFAULT_FILE_READER_CLS as ORIGINAL_DEFAULT_FILE_READER_CLS,
)
from pydantic import BaseModel, Field

from private_gpt.components.embedding.embedding_component import EmbeddingComponent
from private_gpt.components.llm.llm_component import LLMComponent
from private_gpt.components.node_store.node_store_component import NodeStoreComponent
from private_gpt.components.vector_store.vector_store_component import (
    VectorStoreComponent,
)
from private_gpt.paths import local_data_path

if TYPE_CHECKING:
    from llama_index.readers.base import BaseReader

# Patching the default file reader to support other file types
DEFAULT_FILE_READER_CLS = ORIGINAL_DEFAULT_FILE_READER_CLS.copy()
DEFAULT_FILE_READER_CLS.update(
    {
        ".json": JSONReader,
    }
)

logger = logging.getLogger(__name__)



Review comment (suggestion):
Small detail to make it more clear for future readers: instead of patching the default file reader we could:
- Keep on importing DEFAULT_FILE_READER_CLS as it was before
- Create our own FILE_READER_CLS var:
FILE_READER_CLS = DEFAULT_FILE_READER_CLS.copy()
FILE_READER_CLS.update(
    {
        ".json": JSONReader,
    }
)

- Use our FILE_READER_CLS in the ingest function body.

Code after:
import logging
import tempfile
from pathlib import Path
from typing import TYPE_CHECKING, Any, AnyStr

from injector import inject, singleton
from llama_index import (
    Document,
    JSONReader,
    ServiceContext,
    StorageContext,
    StringIterableReader,
    VectorStoreIndex,
    load_index_from_storage,
)
from llama_index.node_parser import SentenceWindowNodeParser
from llama_index.readers.file.base import DEFAULT_FILE_READER_CLS
from pydantic import BaseModel, Field

from private_gpt.components.embedding.embedding_component import EmbeddingComponent
from private_gpt.components.llm.llm_component import LLMComponent
from private_gpt.components.node_store.node_store_component import NodeStoreComponent
from private_gpt.components.vector_store.vector_store_component import (
    VectorStoreComponent,
)
from private_gpt.paths import local_data_path

if TYPE_CHECKING:
    from llama_index.readers.base import BaseReader

# Patching the default file reader to support other file types
FILE_READER_CLS = DEFAULT_FILE_READER_CLS.copy()
FILE_READER_CLS.update(
    {
        ".json": JSONReader,
    }
)

logger = logging.getLogger(__name__)


class IngestedDoc(BaseModel):
    object: str = Field(enum=["ingest.document"])