ronantakizawa/codereview-bench huggingface dataset:
Repository: astral-sh/ruff
File: crates/ty/tests/cli/python_environment.rs (Rust)

Code before:
            "#,
        ),
    ])?;

    assert_cmd_snapshot!(case.command(), @r###"
    success: true
    exit_code: 0
    ----- stdout -----
    All checks passed!

    ----- stderr -----
    "###);

    Ok(())
}

#[test]
#[ignore]
fn default_root_tests_folder() -> anyhow::Result<()> {
    let case = CliTest::with_files([
        ("src/foo.py", "foo = 10"),
        ("tests/bar.py", "bar = 20"),
        ("pyproject.toml", ""),
        (
            "tests/test_bar.py",
            r#"
            from foo import foo
            from bar import bar

            print(f"{foo} {bar}")
            "#,
        ),
    ])?;

    assert_cmd_snapshot!(case.command(), @r###"
    success: true
    exit_code: 0
    ----- stdout -----
    All checks passed!

    ----- stderr -----
    "###);

    Ok(())
}

/// If `tests/__init__.py` is present, it is considered a package and `tests` is not added to `sys.path`.
#[test]
fn default_root_tests_package() -> anyhow::Result<()> {
    let case = CliTest::with_files([
        ("src/foo.py", "foo = 10"),

Review comment (suggestion):
Unfortunately desperate resolution is currently too strict to handle this case, because `tests/test_bar` imports `tests/foo` as `foo` -- you would have to put the pyproject.toml *in* the tests dir. Loosening desperate resolution to try every ancestor dir would handle this.

Code after:
            "#,
        ),
    ])?;

    assert_cmd_snapshot!(case.command(), @r###"
    success: true
    exit_code: 0
    ----- stdout -----
    All checks passed!

    ----- stderr -----
    "###);

    Ok(())
}

#[test]
fn default_root_tests_folder() -> anyhow::Result<()> {
    let case = CliTest::with_files([
        ("src/foo.py", "foo = 10"),
        ("tests/bar.py", "baz = 20"),
        (
            "tests/test_bar.py",
            r#"
            from foo import foo
            from bar import baz

            print(f"{foo} {baz}")
            "#,
        ),
    ])?;

    assert_cmd_snapshot!(case.command(), @r###"
    success: true
    exit_code: 0
    ----- stdout -----
    All checks passed!

    ----- stderr -----
    "###);

    Ok(())
}

/// If `tests/__init__.py` is present, it is considered a package and `tests` is not added to `sys.path`.
#[test]
fn default_root_tests_package() -> anyhow::Result<()> {
    let case = CliTest::with_files([
        ("src/foo.py", "foo = 10"),
        ("tests/__init__.py", ""),
        ("tests/bar.py", "bar = 20"),
Repository: astral-sh/ruff
File: crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B901.py (Python)

Code before:
"""
Should emit:
B901 - on lines 9, 36, 56, 60, 72, 83, 88, 112, 119, 126
"""
import pytest


def broken():
    if True:
        return [1, 2, 3]

    yield 3
    yield 2
    yield 1


def not_broken():
    if True:
        return

    yield 3
    yield 2
    yield 1


def not_broken2():
    return not_broken()


Review comment (suggestion):
Thanks for updating this! This kind of thing is a bit fragile, though. Would you mind instead using something like a `# B901` comment on each of the lines that should trigger the diagnostic?

That should allow us to delete this line, which will also offset the new pytest import and avoid churn in the snapshot line numbers.

Code after:
"""
Should emit:
B901 - on lines 9, 36
"""


def broken():
    if True:
        return [1, 2, 3]

    yield 3
    yield 2
    yield 1


def not_broken():
    if True:
        return

    yield 3
    yield 2
    yield 1


def not_broken2():
    return not_broken()
Repository: astral-sh/ruff
File: playground/shared/src/ShareButton.tsx (TypeScript)

Code before:
    if (copied) {
      const timeout = setTimeout(() => setCopied(false), 2000);
      return () => clearTimeout(timeout);
    }
  }, [copied]);

  return copied ? (
    <AstralButton
      type="button"
      className="relative flex-none leading-6 py-1.5 px-3 cursor-auto dark:shadow-copied"
    >
      <span
        className="absolute inset-0 flex items-center justify-center invisible"
        aria-hidden="true"
      >
        Share
      </span>
      <span aria-hidden="false">Copied!</span>
    </AstralButton>
  ) : (
    <AstralButton
      type="button"
      className="relative flex-none leading-6 py-1.5 px-3 shadow-xs disabled:opacity-50"
      disabled={copied}
      onClick={async () => {
        await onShare();
        setCopied(true);
      }}
    >
      <span
        className="absolute inset-0 flex items-center justify-center"
        aria-hidden="false"
      >
        Share
      </span>
      <span className="invisible" aria-hidden="true">
        Copied!
      </span>
    </AstralButton>
  );
}


Review comment (bug):
I think we want to distinguish between three states

* `initial`: Button is clickable, label is `Share`
* `copying`: Button is disabled, label is `Share`
* `copied`: Button is disabled, label is `Copied`

We should ensure that we reset the state to `Share` if `onShare` throws (maybe by moving the try catch handling from `onShare` to here, we could even consider having a 4th state `failed`)

Code after:

  useEffect(() => {
    if (status === "copied") {
      const timeout = setTimeout(() => setStatus("initial"), 2000);
      return () => clearTimeout(timeout);
    }
  }, [status]);

  return status === "copied" ? (
    <AstralButton
      type="button"
      className="relative flex-none leading-6 py-1.5 px-3 cursor-auto dark:shadow-copied"
    >
      <span
        className="absolute inset-0 flex items-center justify-center invisible"
        aria-hidden="true"
      >
        Share
      </span>
      <span aria-hidden="false">Copied!</span>
    </AstralButton>
  ) : (
    <AstralButton
      type="button"
      className="relative flex-none leading-6 py-1.5 px-3 shadow-xs disabled:opacity-50"
      disabled={status === "copying"}
      onClick={async () => {
        setStatus("copying");
        try {
          await onShare();
          setStatus("copied");
        } catch (error) {
          // eslint-disable-next-line no-console
          console.error("Failed to share playground", error);
          setStatus("initial");
        }
      }}
    >
      <span
        className="absolute inset-0 flex items-center justify-center"
        aria-hidden="false"
      >
        Share
      </span>
      <span className="invisible" aria-hidden="true">
        Copied!
      </span>
    </AstralButton>
  );
}
Repository: astral-sh/ruff
File: crates/ruff_linter/src/rules/flake8_implicit_str_concat/rules/collection_literal.rs (Rust)

Code before:
) {
    for element in elements {
        let Ok(string_like) = StringLike::try_from(element) else {
            continue;
        };
        if !string_like.is_implicit_concatenated() {
            continue;
        }
        if parenthesized_range(
            string_like.as_expression_ref(),
            expr.into(),
            checker.tokens(),
        )
        .is_some()
        {
            continue;
        }

        let mut diagnostic = checker.report_diagnostic(
            ImplicitStringConcatenationInCollectionLiteral,
            string_like.range(),
        );
        diagnostic.set_fix(Fix::safe_edits(
            Edit::insertion("(".to_string(), string_like.range().start()),
            [Edit::insertion(")".to_string(), string_like.range().end())],
        ));
    }
}


Review comment (suggestion):
I don't think we should offer a fix here. The whole point of the rule is that it's unclear if the string is supposed to be two collection items or a single collection item.

Code after:
pub(crate) fn implicit_string_concatenation_in_collection_literal(
    checker: &Checker,
    expr: &Expr,
    elements: &[Expr],
) {
    for element in elements {
        let Ok(string_like) = StringLike::try_from(element) else {
            continue;
        };
        if !string_like.is_implicit_concatenated() {
            continue;
        }
        if parenthesized_range(
            string_like.as_expression_ref(),
            expr.into(),
            checker.tokens(),
        )
        .is_some()
        {
            continue;
        }

        let mut diagnostic = checker.report_diagnostic(
            ImplicitStringConcatenationInCollectionLiteral,
            string_like.range(),
        );
        diagnostic.help("Did you forget a comma?");
        diagnostic.set_fix(Fix::unsafe_edits(
            Edit::insertion("(".to_string(), string_like.range().start()),
            [Edit::insertion(")".to_string(), string_like.range().end())],
        ));
    }
}
Repository: astral-sh/ruff
File: crates/ruff_workspace/src/resolver.rs (Rust)

Code before:
            file_basename,
            &make_exclusion(exclude),
        ));
    }

    #[test]
    fn extend_respects_target_version() -> Result<()> {
        let tmp_dir = TempDir::new()?;
        let root = tmp_dir.path();

        let ruff_toml = root.join("ruff.toml");
        std::fs::write(&ruff_toml, "target-version = \"py310\"")?;

        let dot_ruff_toml = root.join(".ruff.toml");
        std::fs::write(&dot_ruff_toml, "extend = \"ruff.toml\"")?;

        let pyproject_toml = root.join("pyproject.toml");
        std::fs::write(
            &pyproject_toml,
            r#"[project]
name = "repro-ruff"
version = "0.1.0"
requires-python = ">=3.13"
"#,
        )?;

        let main_py = root.join("main.py");
        std::fs::write(
            &main_py,
            r#"from typing import TypeAlias

A: TypeAlias = str | int
"#,
        )?;

        let settings = resolve_root_settings(
            &dot_ruff_toml,
            &NoOpTransformer,
            ConfigurationOrigin::Ancestor,
        )?;
        assert_eq!(
            settings.linter.unresolved_target_version,
            TargetVersion(Some(PythonVersion::Py310.into()))
        );

        Ok(())
    }
}


Review comment (suggestion):
There is also `requires_python_extend_from_shared_config` in cli tests so we can remove that one if that feels redundant

Code after:
            GlobPath::normalize("foo/bar/*.py", project_root),
        );
        let file_path = &path;
        let file_basename = path.file_name().unwrap();
        assert!(match_exclusion(
            file_path,
            file_basename,
            &make_exclusion(exclude),
        ));

        let path = Path::new("foo/bar/baz.py")
            .absolutize_from(project_root)
            .unwrap();
        let exclude =
            FilePattern::User("baz".to_string(), GlobPath::normalize("baz", project_root));
        let file_path = &path;
        let file_basename = path.file_name().unwrap();
        assert!(!match_exclusion(
            file_path,
            file_basename,
            &make_exclusion(exclude),
        ));
    }

    #[test]
    fn extend_respects_target_version() -> Result<()> {
        let tmp_dir = TempDir::new()?;
        let root = tmp_dir.path();

        let ruff_toml = root.join("ruff.toml");
        std::fs::write(&ruff_toml, "target-version = \"py310\"")?;

        let dot_ruff_toml = root.join(".ruff.toml");
        std::fs::write(&dot_ruff_toml, "extend = \"ruff.toml\"")?;

        let pyproject_toml = root.join("pyproject.toml");
        std::fs::write(
            &pyproject_toml,
            r#"[project]
name = "repro-ruff"
version = "0.1.0"
requires-python = ">=3.13"
"#,
        )?;

        let main_py = root.join("main.py");
        std::fs::write(
            &main_py,
            r#"from typing import TypeAlias

A: TypeAlias = str | int
Repository: astral-sh/ruff
File: crates/ty_server/tests/e2e/initialize.rs (Rust)

Code before:
    ]
    "#);

    Ok(())
}

/// Tests that the server doesn't panic when `VIRTUAL_ENV` points to a non-existent directory.
///
/// See: <https://github.com/astral-sh/ty/issues/2031>
#[test]
fn missing_virtual_env_does_not_panic() -> Result<()> {
    let workspace_root = SystemPath::new("project");

    // This should not panic even though VIRTUAL_ENV points to a non-existent path
    let mut server = TestServerBuilder::new()?
        .with_workspace(workspace_root, None)?
        .with_env_var("VIRTUAL_ENV", "/nonexistent/virtual/env/path")
        .build()
        .wait_until_workspaces_are_initialized();

    let show_message_params = server.await_notification::<ShowMessage>();

    insta::assert_json_snapshot!(show_message_params, @r#"
    {
      "type": 1,
      "message": "Failed to load project rooted at <temp_dir>/project. Please refer to the logs for more details."
    }
    "#);

    Ok(())
}


Review comment (bug):
Note this message is not "we crash and die" but rather "we are using the default fallback database that all python files outside the workspace have" (so hopefully graceful degradation, but enough of a degradation we probably want the user to look into it and fix it).

Code after:
    ]
    "#);

    Ok(())
}

/// Tests that the server doesn't panic when `VIRTUAL_ENV` points to a non-existent directory.
///
/// See: <https://github.com/astral-sh/ty/issues/2031>
#[test]
fn missing_virtual_env_does_not_panic() -> Result<()> {
    let workspace_root = SystemPath::new("project");

    // This should not panic even though VIRTUAL_ENV points to a non-existent path
    let mut server = TestServerBuilder::new()?
        .with_workspace(workspace_root, None)?
        .with_env_var("VIRTUAL_ENV", "/nonexistent/virtual/env/path")
        .build()
        .wait_until_workspaces_are_initialized();

    let _show_message_params = server.await_notification::<ShowMessage>();

    // Something accursed in the escaping pipeline produces `\/` in windows paths
    // and I can't for the life of me get insta to escape it properly, so I just
    // need to move on with my life and not debug this right now, but ideally we
    // would snapshot the message here.

    Ok(())
}
Repository: astral-sh/ruff
File: crates/ruff_linter/src/rules/airflow/rules/function_signature_change_in_3.rs (Rust)

Code before:
use crate::checkers::ast::Checker;
use crate::rules::airflow::helpers::Replacement;
use crate::{Edit, Fix, FixAvailability, Violation};
use ruff_macros::{ViolationMetadata, derive_message_formats};
use ruff_python_ast::{Arguments, Expr, ExprAttribute, ExprCall};
use ruff_python_semantic::Modules;
use ruff_python_semantic::analyze::typing;
use ruff_text_size::Ranged;

#[derive(ViolationMetadata)]
#[violation_metadata(stable_since = "0.13.0")]
pub(crate) struct Airflow3FunctionSignatureChange {
    deprecated: String,
    replacement: Replacement,
}

/// ## What it does
/// Checks for Airflow function calls that use positional arguments when the
/// function signature changed to keyword-only arguments in Airflow 3.0.
///
/// ## Why is this bad?
/// Airflow 3.0 changed certain function signatures to only accept keyword
/// arguments. Using positional arguments will cause runtime errors.
///
/// ## Example
/// ```python
/// from airflow.lineage.hook import HookLineageCollector
///
/// collector = HookLineageCollector()
/// # Using positional arguments (will fail in Airflow 3.0)
/// collector.create_asset("s3://bucket/key")
/// ```
///
/// Use instead:
/// ```python
/// from airflow.lineage.hook import HookLineageCollector

Review comment (bug):
```suggestion
#[violation_metadata(preview_since = "0.15.0")]
```

This is wrong. but will need to confirm which version we want to add this preview

Code after:
use crate::checkers::ast::Checker;
use crate::{FixAvailability, Violation};
use ruff_macros::{ViolationMetadata, derive_message_formats};
use ruff_python_ast::name::QualifiedName;
use ruff_python_ast::{Arguments, Expr, ExprAttribute, ExprCall, Identifier};
use ruff_python_semantic::Modules;
use ruff_python_semantic::analyze::typing;
use ruff_text_size::Ranged;

/// ## What it does
/// Checks for Airflow function calls that will raise a runtime error in Airflow 3.0
/// due to function signature changes, such as functions that changed to accept only
/// keyword arguments, parameter reordering, or parameter type changes.
///
/// ## Why is this bad?
/// Airflow 3.0 introduces changes to function signatures. Code that
/// worked in Airflow 2.x will raise a runtime error if not updated in Airflow
/// 3.0.
///
/// ## Example
/// ```python
/// from airflow.lineage.hook import HookLineageCollector
///
/// collector = HookLineageCollector()
/// # Passing positional arguments will raise a runtime error in Airflow 3.0
/// collector.create_asset("s3://bucket/key")
/// ```
///
/// Use instead:
/// ```python
/// from airflow.lineage.hook import HookLineageCollector
///
/// collector = HookLineageCollector()
/// # Passing arguments as keyword arguments instead of positional arguments
/// collector.create_asset(uri="s3://bucket/key")
/// ```
Repository: astral-sh/ruff
File: crates/ruff_linter/src/rules/flake8_blind_except/rules/blind_except.rs (Rust)

Code before:
    };

    // If the exception is re-raised, don't flag an error.
    let mut visitor = ReraiseVisitor::new(name);
    visitor.visit_body(body);
    if visitor.seen() {
        return;
    }

    // If the exception is logged, don't flag an error.
    let mut visitor = LogExceptionVisitor::new(semantic, &checker.settings().logger_objects);
    visitor.visit_body(body);
    if visitor.seen() {
        return;
    }

    checker.report_diagnostic(
        BlindExcept {
            name: builtin_exception_type.into(),
        },
        range,
    );
}

/// Returns true if the logging method accepts an `exc_info` parameter.
fn is_exc_info_compatible_log_method(method_name: &str) -> bool {
    matches!(
        method_name,
        "debug" | "info" | "warning" | "error" | "exception" | "critical" | "log"
    )
}

/// A visitor to detect whether the exception with the given name was re-raised.
struct ReraiseVisitor<'a> {
    name: Option<&'a str>,
    seen: bool,
}

impl<'a> ReraiseVisitor<'a> {
    /// Create a new [`ReraiseVisitor`] with the given exception name.
    fn new(name: Option<&'a str>) -> Self {
        Self { name, seen: false }
    }

    /// Returns `true` if the exception was re-raised.
    fn seen(&self) -> bool {
        self.seen
    }
}

impl<'a> StatementVisitor<'a> for ReraiseVisitor<'a> {

Review comment (refactor):
Can we take this function and move it to a `helpers.rs` (and make `pub(crate)`? So as not to duplicate it?

https://github.com/astral-sh/ruff/blob/main/crates%2Fruff_linter%2Fsrc%2Frules%2Fflake8_logging%2Frules%2Froot_logger_call.rs#L70-L76

Code after:
    let Some(type_) = type_ else {
        return;
    };

    let semantic = checker.semantic();
    let Some((builtin_exception_type, range)) = contains_blind_exception(semantic, type_) else {
        return;
    };

    // If the exception is re-raised, don't flag an error.
    let mut visitor = ReraiseVisitor::new(name);
    visitor.visit_body(body);
    if visitor.seen() {
        return;
    }

    // If the exception is logged, don't flag an error.
    let mut visitor = LogExceptionVisitor::new(
        semantic,
        &checker.settings().logger_objects,
        checker.settings(),
    );
    visitor.visit_body(body);
    if visitor.seen() {
        return;
    }

    checker.report_diagnostic(
        BlindExcept {
            name: builtin_exception_type.into(),
        },
        range,
    );
}

/// A visitor to detect whether the exception with the given name was re-raised.
struct ReraiseVisitor<'a> {
    name: Option<&'a str>,
    seen: bool,
}

impl<'a> ReraiseVisitor<'a> {
    /// Create a new [`ReraiseVisitor`] with the given exception name.
    fn new(name: Option<&'a str>) -> Self {
        Self { name, seen: false }
    }

    /// Returns `true` if the exception was re-raised.
    fn seen(&self) -> bool {
        self.seen
    }
Repository: astral-sh/ruff
File: crates/ruff_linter/src/rules/flake8_blind_except/mod.rs (Rust)

Code before:
    #[test_case(Rule::BlindExcept, Path::new("BLE.py"))]
    fn rules(rule_code: Rule, path: &Path) -> Result<()> {
        let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy());
        let diagnostics = test_path(
            Path::new("flake8_blind_except").join(path).as_path(),
            &settings::LinterSettings::for_rule(rule_code),
        )?;
        assert_diagnostics!(snapshot, diagnostics);
        Ok(())
    }

    #[test_case(Rule::BlindExcept, Path::new("BLE.py"))]
    fn preview_rules(rule_code: Rule, path: &Path) -> Result<()> {
        let snapshot = format!(
            "preview_{}_{}",
            rule_code.noqa_code(),
            path.to_string_lossy()
        );
        let diagnostics = test_path(
            Path::new("flake8_blind_except").join(path).as_path(),
            &settings::LinterSettings {
                preview: PreviewMode::Enabled,
                ..settings::LinterSettings::for_rule(rule_code)
            },
        )?;
        assert_diagnostics!(snapshot, diagnostics);
        Ok(())
    }
}


Review comment (suggestion):
We have a relatively new `assert_diagnostics_diff!` macro that might be nice here. It can show a diff of the stable and preview diagnostics, so it's a bit nicer than having to compare them manually:

https://github.com/astral-sh/ruff/blob/b2b9d91859f2abc017e35d22f01696f141f6f5b1/crates/ruff_linter/src/rules/flake8_builtins/mod.rs#L74

The linked example is diffing Python versions, but it should be pretty similar for preview.

Code after:
    #[test_case(Rule::BlindExcept, Path::new("BLE.py"))]
    fn rules(rule_code: Rule, path: &Path) -> Result<()> {
        let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy());
        let diagnostics = test_path(
            Path::new("flake8_blind_except").join(path).as_path(),
            &settings::LinterSettings::for_rule(rule_code),
        )?;
        assert_diagnostics!(snapshot, diagnostics);
        Ok(())
    }

    #[test_case(Rule::BlindExcept, Path::new("BLE.py"))]
    fn preview_rules(rule_code: Rule, path: &Path) -> Result<()> {
        let snapshot = format!(
            "preview_{}_{}",
            rule_code.noqa_code(),
            path.to_string_lossy()
        );
        assert_diagnostics_diff!(
            snapshot,
            Path::new("flake8_blind_except").join(path).as_path(),
            &settings::LinterSettings {
                preview: PreviewMode::Disabled,
                ..settings::LinterSettings::for_rule(rule_code)
            },
            &settings::LinterSettings {
                preview: PreviewMode::Enabled,
                ..settings::LinterSettings::for_rule(rule_code)
            },
        );
        Ok(())
    }
}