ronantakizawa/codereview-bench huggingface dataset:
Repository: astral-sh/ruff
File: scripts/conformance.py (Python)

Code before:

    def into_title(self) -> str:
        match self:
            case Classification.TRUE_POSITIVE:
                return "True positives added"
            case Classification.FALSE_POSITIVE:
                return "False positives added"
            case Classification.TRUE_NEGATIVE:
                return "False positives removed"
            case Classification.FALSE_NEGATIVE:
                return "True positives removed"


class Change(StrEnum):
    ADDED = auto()
    REMOVED = auto()
    UNCHANGED = auto()

    def into_title(self) -> str:
        match self:
            case Change.ADDED:
                return "Added (Optional)"
            case Change.REMOVED:
                return "Removed (Optional)"
            case Change.UNCHANGED:
                return "Unchanged (Optional)"


@dataclass(kw_only=True, slots=True)
class Position:
    line: int
    column: int


@dataclass(kw_only=True, slots=True)
class Positions:
    begin: Position
    end: Position


@dataclass(kw_only=True, slots=True)
class Location:
    path: Path
    positions: Positions

    def as_link(self) -> str:
        file = self.path.name
        link = CONFORMANCE_URL.format(
            conformance_suite_commit=CONFORMANCE_SUITE_COMMIT,
            filename=file,
            line=self.positions.begin.line,

Review comment (suggestion):
```suggestion
                return "Optional Diagnostics Added"
            case Change.REMOVED:
                return "Optional Diagnostics Removed"
            case Change.UNCHANGED:
                return "Optional Diagnostics Unchanged"
```

Code after:

    def into_title(self) -> str:
        match self:
            case Classification.TRUE_POSITIVE:
                return "True positives added"
            case Classification.FALSE_POSITIVE:
                return "False positives added"
            case Classification.TRUE_NEGATIVE:
                return "False positives removed"
            case Classification.FALSE_NEGATIVE:
                return "True positives removed"


class Change(StrEnum):
    ADDED = auto()
    REMOVED = auto()
    UNCHANGED = auto()

    def into_title(self) -> str:
        match self:
            case Change.ADDED:
                return "Optional Diagnostics Added"
            case Change.REMOVED:
                return "Optional Diagnostics Removed"
            case Change.UNCHANGED:
                return "Optional Diagnostics Unchanged"


@dataclass(kw_only=True, slots=True)
class Position:
    line: int
    column: int


@dataclass(kw_only=True, slots=True)
class Positions:
    begin: Position
    end: Position


@dataclass(kw_only=True, slots=True)
class Location:
    path: Path
    positions: Positions

    def as_link(self) -> str:
        file = self.path.name
        link = CONFORMANCE_URL.format(
            conformance_suite_commit=CONFORMANCE_SUITE_COMMIT,
            filename=file,
            line=self.positions.begin.line,
Repository: astral-sh/ruff
File: scripts/conformance.py (Python)

Code before:
                        source=Source.EXPECTED,
                        optional=error.group("optional") is not None,
                    )
                )

    assert diagnostics, "Failed to discover any expected diagnostics!"
    return diagnostics


def collect_ty_diagnostics(
    ty_path: list[str],
    source: Source,
    tests_path: str = ".",
    python_version: str = "3.12",
) -> list[Diagnostic]:
    process = subprocess.run(
        [
            *ty_path,
            "check",
            f"--python-version={python_version}",
            "--output-format=gitlab",
            "--exit-zero",
            *(
                str(path)
                for path in Path(tests_path).iterdir()
                if path.suffix in {".py", ".pyi"} and not path.name.startswith("_")
            ),
        ],
        capture_output=True,
        text=True,
        check=True,
        timeout=15,
    )

    if process.returncode != 0:
        print(process.stderr)
        raise RuntimeError(f"ty check failed with exit code {process.returncode}")

    return [
        Diagnostic.from_gitlab_output(dct, source=source)
        for dct in json.loads(process.stdout)
        if dct["severity"] == "major"
    ]


def group_diagnostics_by_key(
    old: list[Diagnostic], new: list[Diagnostic], expected: list[Diagnostic]
) -> list[GroupedDiagnostics]:
    diagnostics = [
        *old,
        *new,

Review comment (refactor):
Should we create a helper method that, given a path, returns whether this is a conformance test file?

Code after:
                    )
                )

    assert diagnostics, "Failed to discover any expected diagnostics!"
    return diagnostics


def collect_ty_diagnostics(
    ty_path: list[str],
    source: Source,
    test_files: Sequence[Path],
    python_version: str = "3.12",
) -> list[Diagnostic]:
    process = subprocess.run(
        [
            *ty_path,
            "check",
            f"--python-version={python_version}",
            "--output-format=gitlab",
            "--exit-zero",
            *map(str, test_files),
        ],
        capture_output=True,
        text=True,
        check=True,
        timeout=15,
    )

    if process.returncode != 0:
        print(process.stderr)
        raise RuntimeError(f"ty check failed with exit code {process.returncode}")

    return [
        Diagnostic.from_gitlab_output(dct, source=source)
        for dct in json.loads(process.stdout)
        if dct["severity"] == "major"
    ]


def group_diagnostics_by_key(
    old: list[Diagnostic], new: list[Diagnostic], expected: list[Diagnostic]
) -> list[GroupedDiagnostics]:
    diagnostics = [
        *old,
        *new,
        *expected,
    ]

    sorted_diagnostics = sorted(diagnostics, key=attrgetter("key"))

    grouped = []
Repository: astral-sh/ruff
File: crates/ty_test/src/matcher.rs (Rust)

Code before:
                    r#"unexpected error: 1 [some-rule] "some message""#,
                ],
            )],
        );
    }

    #[test]
    fn trailing_quote_without_message_not_allowed() {
        let source = "x  # error: [some-rule]\"";
        let result = get_result(
            source,
            vec![ExpectedDiagnostic::new(
                DiagnosticId::lint("some-rule"),
                "some message",
                0,
            )],
        );

        assert_fail(
            result,
            &[(
                0,
                &[
                    "invalid assertion: expected '\"' to be the final character in an assertion with an error message",
                    r#"unexpected error: 1 [some-rule] "some message""#,
                ],
            )],
        );
    }
}


Review comment (suggestion):
hmm, should we add a new `ErrorAssertionParseError` variant for the specific case where the assertion _does_ end with `"` as the final character, but the quotes were unclosed? This error message feels a bit confusing for this specific case, because `"` _was_ the final character here, after all!

Code after:
                    r#"unexpected error: 1 [some-rule] "some message""#,
                ],
            )],
        );
    }

    #[test]
    fn trailing_quote_without_message_not_allowed() {
        let source = "x  # error: [some-rule]\"";
        let result = get_result(
            source,
            vec![ExpectedDiagnostic::new(
                DiagnosticId::lint("some-rule"),
                "some message",
                0,
            )],
        );

        assert_fail(
            result,
            &[(
                0,
                &[
                    "invalid assertion: expected message text and closing '\"' after opening '\"'",
                    r#"unexpected error: 1 [some-rule] "some message""#,
                ],
            )],
        );
    }
}
Repository: astral-sh/ruff
File: scripts/conformance.py (Python)

Code before:


class Source(Flag):
    OLD = auto()
    NEW = auto()
    EXPECTED = auto()


class Classification(StrEnum):
    TRUE_POSITIVE = auto()
    FALSE_POSITIVE = auto()
    TRUE_NEGATIVE = auto()
    FALSE_NEGATIVE = auto()

    def into_title(self) -> str:
        match self:
            case Classification.TRUE_POSITIVE:
                return "True positives added"
            case Classification.FALSE_POSITIVE:
                return "False positives added"
            case Classification.TRUE_NEGATIVE:
                return "False positives removed"
            case Classification.FALSE_NEGATIVE:
                return "True positives removed"


class Change(StrEnum):
    ADDED = auto()
    REMOVED = auto()
    UNCHANGED = auto()

    def into_title(self) -> str:
        match self:
            case Change.ADDED:
                return "Optional Diagnostics Added"
            case Change.REMOVED:
                return "Optional Diagnostics Removed"
            case Change.UNCHANGED:
                return "Optional Diagnostics Unchanged"


@dataclass(kw_only=True, slots=True)
class Position:
    line: int
    column: int


@dataclass(kw_only=True, slots=True)
class Positions:
    begin: Position
    end: Position

Review comment (suggestion):
This class feels pretty heavy only to support the case where one group has both true and false positives. 

I was wondering if we could change `classify` to return an iterable of `(Classification, int)` instead. Most groups return exactly one, with the exception of the `many` case where ty emits too many diagnostics, in which case we return two.

Code after:
class Source(StrEnum):
    OLD = auto()
    NEW = auto()
    EXPECTED = auto()


class Classification(StrEnum):
    TRUE_POSITIVE = auto()
    FALSE_POSITIVE = auto()
    TRUE_NEGATIVE = auto()
    FALSE_NEGATIVE = auto()

    def into_title(self) -> str:
        match self:
            case Classification.TRUE_POSITIVE:
                return "True positives added"
            case Classification.FALSE_POSITIVE:
                return "False positives added"
            case Classification.TRUE_NEGATIVE:
                return "False positives removed"
            case Classification.FALSE_NEGATIVE:
                return "True positives removed"


@dataclass(kw_only=True, slots=True)
class Evaluation:
    classification: Classification
    true_positives: int = 0
    false_positives: int = 0
    true_negatives: int = 0
    false_negatives: int = 0


class Change(StrEnum):
    ADDED = auto()
    REMOVED = auto()
    UNCHANGED = auto()

    def into_title(self) -> str:
        match self:
            case Change.ADDED:
                return "Optional Diagnostics Added"
            case Change.REMOVED:
                return "Optional Diagnostics Removed"
            case Change.UNCHANGED:
                return "Optional Diagnostics Unchanged"


@dataclass(kw_only=True, slots=True)
class Position:
    line: int
Repository: astral-sh/ruff
File: crates/ruff_linter/src/rules/refurb/rules/if_exp_instead_of_or_operator.rs (Rust)

Code before:

/// ## What it does
/// Checks for ternary `if` expressions that can be replaced with the `or`
/// operator.
///
/// ## Why is this bad?
/// Ternary `if` expressions are more verbose than `or` expressions while
/// providing the same functionality.
///
/// ## Example
/// ```python
/// x, y = 1, 2
///
/// z = x if x else y
/// ```
///
/// Use instead:
/// ```python
/// x, y = 1, 2
///
/// z = x or y
/// ```
///
/// ## Fix safety
/// This rule's fix is marked as unsafe in the event that the body of the
/// `if` expression contains side effects, or if it removes comments.
///
/// For example, `foo` will be called twice in `foo() if foo() else bar()`
/// (assuming `foo()` returns a truthy value), but only once in
/// `foo() or bar()`.
#[derive(ViolationMetadata)]
#[violation_metadata(preview_since = "v0.3.6")]
pub(crate) struct IfExpInsteadOfOrOperator;

impl Violation for IfExpInsteadOfOrOperator {
    const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes;

    #[derive_message_formats]
    fn message(&self) -> String {
        "Replace ternary `if` expression with `or` operator".to_string()
    }

    fn fix_title(&self) -> Option<String> {
        Some("Replace with `or` operator".to_string())
    }
}

/// FURB110
pub(crate) fn if_exp_instead_of_or_operator(checker: &Checker, if_expr: &ast::ExprIf) {
    let ast::ExprIf {
        test,

Review comment (suggestion):
Small nit, but the other part of the sentence refers to the `body` of the expression, so I think it's confusing for the `it` here to refer to the fix instead.


```suggestion
/// `if` expression contains side effects or comments.
```

Code after:

/// ## What it does
/// Checks for ternary `if` expressions that can be replaced with the `or`
/// operator.
///
/// ## Why is this bad?
/// Ternary `if` expressions are more verbose than `or` expressions while
/// providing the same functionality.
///
/// ## Example
/// ```python
/// x, y = 1, 2
///
/// z = x if x else y
/// ```
///
/// Use instead:
/// ```python
/// x, y = 1, 2
///
/// z = x or y
/// ```
///
/// ## Fix safety
/// This rule's fix is marked as unsafe in the event that the body of the
/// `if` expression contains side effects or comments.
///
/// For example, `foo` will be called twice in `foo() if foo() else bar()`
/// (assuming `foo()` returns a truthy value), but only once in
/// `foo() or bar()`.
#[derive(ViolationMetadata)]
#[violation_metadata(preview_since = "v0.3.6")]
pub(crate) struct IfExpInsteadOfOrOperator;

impl Violation for IfExpInsteadOfOrOperator {
    const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes;

    #[derive_message_formats]
    fn message(&self) -> String {
        "Replace ternary `if` expression with `or` operator".to_string()
    }

    fn fix_title(&self) -> Option<String> {
        Some("Replace with `or` operator".to_string())
    }
}

/// FURB110
pub(crate) fn if_exp_instead_of_or_operator(checker: &Checker, if_expr: &ast::ExprIf) {
    let ast::ExprIf {
        test,
Repository: astral-sh/ruff
File: crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI034.py (Python)

Code before:

def __str__(self) -> str:
    ...


def __eq__(self, other: Any) -> bool:
    ...


def __ne__(self, other: Any) -> bool:
    ...


def __imul__(self, other: Any) -> list[str]:
    ...

class UsesStringizedAnnotations:
    def __iadd__(self, other: "UsesStringizedAnnotations") -> "typing.Self":
        return self


class UsesStringizedForwardReferences:
    def __new__(cls) -> "UsesStringizedForwardReferences": ...       # PYI034
    def __enter__(self) -> "UsesStringizedForwardReferences": ...    # PYI034
    async def __aenter__(self) -> "UsesStringizedForwardReferences": ...  # PYI034
    def __iadd__(self, other) -> "UsesStringizedForwardReferences": ...  # PYI034


class NonGeneric1(tuple):
    def __new__(cls: type[NonGeneric1], *args, **kwargs) -> NonGeneric1: ...
    def __enter__(self: NonGeneric1) -> NonGeneric1: ...

class NonGeneric2(tuple):
    def __new__(cls: Type[NonGeneric2]) -> NonGeneric2: ...

class Generic1[T](list):
    def __new__(cls: type[Generic1]) -> Generic1: ...
    def __enter__(self: Generic1) -> Generic1: ...


### Correctness of typevar-likes are not verified.

T = TypeVar('T')
P = ParamSpec()
Ts = TypeVarTuple('foo')

class Generic2(Generic[T]):
    def __new__(cls: type[Generic2]) -> Generic2: ...
    def __enter__(self: Generic2) -> Generic2: ...

class Generic3(tuple[*Ts]):

Review comment (refactor):
nit: could you possibly move this to the end of the fixture file? It's very hard to review the diff to the snapshot when all the line numbers change 😆

Code after:

def __str__(self) -> str:
    ...


def __eq__(self, other: Any) -> bool:
    ...


def __ne__(self, other: Any) -> bool:
    ...


def __imul__(self, other: Any) -> list[str]:
    ...

class UsesStringizedAnnotations:
    def __iadd__(self, other: "UsesStringizedAnnotations") -> "typing.Self":
        return self


class NonGeneric1(tuple):
    def __new__(cls: type[NonGeneric1], *args, **kwargs) -> NonGeneric1: ...
    def __enter__(self: NonGeneric1) -> NonGeneric1: ...

class NonGeneric2(tuple):
    def __new__(cls: Type[NonGeneric2]) -> NonGeneric2: ...

class Generic1[T](list):
    def __new__(cls: type[Generic1]) -> Generic1: ...
    def __enter__(self: Generic1) -> Generic1: ...


### Correctness of typevar-likes are not verified.

T = TypeVar('T')
P = ParamSpec()
Ts = TypeVarTuple('foo')

class Generic2(Generic[T]):
    def __new__(cls: type[Generic2]) -> Generic2: ...
    def __enter__(self: Generic2) -> Generic2: ...

class Generic3(tuple[*Ts]):
    def __new__(cls: type[Generic3]) -> Generic3: ...
    def __enter__(self: Generic3) -> Generic3: ...

class Generic4(collections.abc.Callable[P, ...]):
    def __new__(cls: type[Generic4]) -> Generic4: ...
    def __enter__(self: Generic4) -> Generic4: ...
Repository: astral-sh/ruff
File: crates/ruff_linter/src/rules/airflow/helpers.rs (Rust)

Code before:
    );

    Some(Fix::unsafe_edits(remove_edit, [import_edit]))
}

/// This is a helper function to check if the given function definition is a method
/// that inherits from a base class.
pub(crate) fn is_method_in_subclass<F>(
    function_def: &StmtFunctionDef,
    semantic: &SemanticModel,
    method_name: &str,
    is_base_class: F,
) -> bool
where
    F: Fn(&[&str]) -> bool,
{
    if function_def.name.as_str() != method_name {
        return false;
    }

    let ScopeKind::Class(class_def) = semantic.current_scope().kind else {
        return false;
    };

    class_def.bases().iter().any(|class_base| {
        semantic
            .resolve_qualified_name(class_base)
            .is_some_and(|qualified_name| is_base_class(qualified_name.segments()))
    })
}


Review comment (refactor):
This helper function generalizes the function `is_execute_method_inherits_from_airflow_operator` defined in the `removal_in_3.rs`. The function is checking if a subclass of airflow's `BaseOperator` implements a `execute` function. In AIR303, we need to check if a subclass of airflow's `BaseOperatorLink` implements a `get_link` function. Therefore, this is something reusable across multiple rules. I parameterized the method name and the match condition, to reduce redundant definition for similar checks.

Code after:
        .or_else(|| checker.semantic().lookup_symbol(&head.id))
        .map(|id| checker.semantic().binding(id))?;
    let stmt = binding.statement(semantic)?;
    let remove_edit = remove_unused_imports(
        std::iter::once(name),
        stmt,
        None,
        checker.locator(),
        checker.stylist(),
        checker.indexer(),
    )
    .ok()?;
    let import_edit = checker.importer().add_import(
        &NameImport::ImportFrom(MemberNameImport::member(
            (*module).to_string(),
            name.to_string(),
        )),
        expr.start(),
    );

    Some(Fix::unsafe_edits(remove_edit, [import_edit]))
}

/// This is a helper function to check if the given function definition is a method
/// that inherits from a base class.
pub(crate) fn is_method_in_subclass<F>(
    function_def: &StmtFunctionDef,
    semantic: &SemanticModel,
    method_name: &str,
    is_base_class: F,
) -> bool
where
    F: Fn(QualifiedName) -> bool,
{
    if function_def.name.as_str() != method_name {
        return false;
    }

    let ScopeKind::Class(class_def) = semantic.current_scope().kind else {
        return false;
    };

    any_qualified_base_class(class_def, semantic, &is_base_class)
}
Repository: astral-sh/ruff
File: crates/ruff_linter/src/rules/airflow/helpers.rs (Rust)

Code before:
}

/// This is a helper function to check if the given function definition is a method
/// that inherits from a base class.
pub(crate) fn is_method_in_subclass<F>(
    function_def: &StmtFunctionDef,
    semantic: &SemanticModel,
    method_name: &str,
    is_base_class: F,
) -> bool
where
    F: Fn(&[&str]) -> bool,
{
    if function_def.name.as_str() != method_name {
        return false;
    }

    let ScopeKind::Class(class_def) = semantic.current_scope().kind else {
        return false;
    };

    class_def.bases().iter().any(|class_base| {
        semantic
            .resolve_qualified_name(class_base)
            .is_some_and(|qualified_name| is_base_class(qualified_name.segments()))
    })
}


Review comment (suggestion):
You may want to use `any_base_class` here:

https://github.com/astral-sh/ruff/blob/0027eaf85ddd107ad22feec8ee4423fa86c82acc/crates/ruff_python_semantic/src/analyze/class.rs#L28-L30

It will recurse in case the base class is itself a base class of `BaseOperator`. I guess `any_qualified_base_class` might be the even better suggestion here.

Code after:
    let remove_edit = remove_unused_imports(
        std::iter::once(name),
        stmt,
        None,
        checker.locator(),
        checker.stylist(),
        checker.indexer(),
    )
    .ok()?;
    let import_edit = checker.importer().add_import(
        &NameImport::ImportFrom(MemberNameImport::member(
            (*module).to_string(),
            name.to_string(),
        )),
        expr.start(),
    );

    Some(Fix::unsafe_edits(remove_edit, [import_edit]))
}

/// This is a helper function to check if the given function definition is a method
/// that inherits from a base class.
pub(crate) fn is_method_in_subclass<F>(
    function_def: &StmtFunctionDef,
    semantic: &SemanticModel,
    method_name: &str,
    is_base_class: F,
) -> bool
where
    F: Fn(QualifiedName) -> bool,
{
    if function_def.name.as_str() != method_name {
        return false;
    }

    let ScopeKind::Class(class_def) = semantic.current_scope().kind else {
        return false;
    };

    any_qualified_base_class(class_def, semantic, &is_base_class)
}
Repository: astral-sh/ruff
File: crates/ruff_markdown/src/lib.rs (Rust)

Code before:
        This is poorly formatted code:

        ```
        print("hello")
        ```
        "#);
    }

    #[test]
    fn format_code_blocks_unlabeled_rust() {
        let code = r#"
This is poorly formatted code:

```
fn (foo: &str) -> &str {
    foo
}
```
        "#;
        assert_snapshot!(
            format_code_blocks(code, None, &FormatterSettings::default()),
            @"Unchanged");
    }

    #[test]
    fn format_code_blocks_tildas() {
        let code = r#"
~~~py
print( 'hello' )
~~~
        "#;
        assert_snapshot!(
            format_code_blocks(code, None, &FormatterSettings::default()),
            @r#"
        ~~~py
        print("hello")
        ~~~
        "#);
    }

    #[test]
    fn format_code_blocks_long_fence() {
        let code = r#"
````py
print( 'hello' )
````
~~~~~py
print( 'hello' )
~~~~~
        "#;
        assert_snapshot!(

Review comment (suggestion):
```suggestion
    fn format_code_blocks_tildes() {
```

Code after:
        "#;
        assert_snapshot!(
            format_code_blocks(code, None, &FormatterSettings::default()),
            @r#"
        This is poorly formatted code:

        ```
        print("hello")
        ```
        "#);
    }

    #[test]
    fn format_code_blocks_unlabeled_rust() {
        let code = r#"
This is poorly formatted code:

```
fn (foo: &str) -> &str {
    foo
}
```
        "#;
        assert_snapshot!(
            format_code_blocks(code, None, &FormatterSettings::default()),
            @"Unchanged");
    }

    #[test]
    fn format_code_blocks_tildes() {
        let code = r#"
~~~py
print( 'hello' )
~~~
        "#;
        assert_snapshot!(
            format_code_blocks(code, None, &FormatterSettings::default()),
            @r#"
        ~~~py
        print("hello")
        ~~~
        "#);
    }

    #[test]
    fn format_code_blocks_long_fence() {
        let code = r#"
````py
print( 'hello' )
````
~~~~~py