Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[flake8-builtins] Rename A005 and improve its error message #15348

Merged
merged 1 commit into from
Jan 8, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions crates/ruff_linter/src/checkers/filesystem.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use ruff_python_trivia::CommentRanges;

use crate::package::PackageRoot;
use crate::registry::Rule;
use crate::rules::flake8_builtins::rules::builtin_module_shadowing;
use crate::rules::flake8_builtins::rules::stdlib_module_shadowing;
use crate::rules::flake8_no_pep420::rules::implicit_namespace_package;
use crate::rules::pep8_naming::rules::invalid_module_name;
use crate::settings::LinterSettings;
Expand Down Expand Up @@ -45,8 +45,8 @@ pub(crate) fn check_file_path(
}

// flake8-builtins
if settings.rules.enabled(Rule::BuiltinModuleShadowing) {
if let Some(diagnostic) = builtin_module_shadowing(
if settings.rules.enabled(Rule::StdlibModuleShadowing) {
if let Some(diagnostic) = stdlib_module_shadowing(
path,
package,
&settings.flake8_builtins.builtins_allowed_modules,
Expand Down
2 changes: 1 addition & 1 deletion crates/ruff_linter/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -317,7 +317,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
(Flake8Builtins, "002") => (RuleGroup::Stable, rules::flake8_builtins::rules::BuiltinArgumentShadowing),
(Flake8Builtins, "003") => (RuleGroup::Stable, rules::flake8_builtins::rules::BuiltinAttributeShadowing),
(Flake8Builtins, "004") => (RuleGroup::Stable, rules::flake8_builtins::rules::BuiltinImportShadowing),
(Flake8Builtins, "005") => (RuleGroup::Preview, rules::flake8_builtins::rules::BuiltinModuleShadowing),
(Flake8Builtins, "005") => (RuleGroup::Preview, rules::flake8_builtins::rules::StdlibModuleShadowing),
(Flake8Builtins, "006") => (RuleGroup::Preview, rules::flake8_builtins::rules::BuiltinLambdaArgumentShadowing),

// flake8-bugbear
Expand Down
2 changes: 1 addition & 1 deletion crates/ruff_linter/src/registry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -306,7 +306,7 @@ impl Rule {
Rule::UnsortedImports | Rule::MissingRequiredImport => LintSource::Imports,
Rule::ImplicitNamespacePackage
| Rule::InvalidModuleName
| Rule::BuiltinModuleShadowing => LintSource::Filesystem,
| Rule::StdlibModuleShadowing => LintSource::Filesystem,
Rule::IndentationWithInvalidMultiple
| Rule::IndentationWithInvalidMultipleComment
| Rule::MissingWhitespace
Expand Down
24 changes: 12 additions & 12 deletions crates/ruff_linter/src/rules/flake8_builtins/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,26 +21,26 @@ mod tests {
#[test_case(Rule::BuiltinAttributeShadowing, Path::new("A003.py"))]
#[test_case(Rule::BuiltinImportShadowing, Path::new("A004.py"))]
#[test_case(
Rule::BuiltinModuleShadowing,
Rule::StdlibModuleShadowing,
Path::new("A005/modules/non_builtin/__init__.py")
)]
#[test_case(
Rule::BuiltinModuleShadowing,
Rule::StdlibModuleShadowing,
Path::new("A005/modules/logging/__init__.py")
)]
#[test_case(
Rule::BuiltinModuleShadowing,
Rule::StdlibModuleShadowing,
Path::new("A005/modules/string/__init__.py")
)]
#[test_case(
Rule::BuiltinModuleShadowing,
Rule::StdlibModuleShadowing,
Path::new("A005/modules/package/bisect.py")
)]
#[test_case(
Rule::BuiltinModuleShadowing,
Rule::StdlibModuleShadowing,
Path::new("A005/modules/_abc/__init__.py")
)]
#[test_case(Rule::BuiltinModuleShadowing, Path::new("A005/modules/package/xml.py"))]
#[test_case(Rule::StdlibModuleShadowing, Path::new("A005/modules/package/xml.py"))]
#[test_case(Rule::BuiltinLambdaArgumentShadowing, Path::new("A006.py"))]
fn rules(rule_code: Rule, path: &Path) -> Result<()> {
let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy());
Expand Down Expand Up @@ -80,26 +80,26 @@ mod tests {
}

#[test_case(
Rule::BuiltinModuleShadowing,
Rule::StdlibModuleShadowing,
Path::new("A005/modules/non_builtin/__init__.py")
)]
#[test_case(
Rule::BuiltinModuleShadowing,
Rule::StdlibModuleShadowing,
Path::new("A005/modules/logging/__init__.py")
)]
#[test_case(
Rule::BuiltinModuleShadowing,
Rule::StdlibModuleShadowing,
Path::new("A005/modules/string/__init__.py")
)]
#[test_case(
Rule::BuiltinModuleShadowing,
Rule::StdlibModuleShadowing,
Path::new("A005/modules/package/bisect.py")
)]
#[test_case(
Rule::BuiltinModuleShadowing,
Rule::StdlibModuleShadowing,
Path::new("A005/modules/_abc/__init__.py")
)]
#[test_case(Rule::BuiltinModuleShadowing, Path::new("A005/modules/package/xml.py"))]
#[test_case(Rule::StdlibModuleShadowing, Path::new("A005/modules/package/xml.py"))]
fn builtins_allowed_modules(rule_code: Rule, path: &Path) -> Result<()> {
let snapshot = format!(
"{}_{}_builtins_allowed_modules",
Expand Down
4 changes: 2 additions & 2 deletions crates/ruff_linter/src/rules/flake8_builtins/rules/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,12 @@ pub(crate) use builtin_argument_shadowing::*;
pub(crate) use builtin_attribute_shadowing::*;
pub(crate) use builtin_import_shadowing::*;
pub(crate) use builtin_lambda_argument_shadowing::*;
pub(crate) use builtin_module_shadowing::*;
pub(crate) use builtin_variable_shadowing::*;
pub(crate) use stdlib_module_shadowing::*;

mod builtin_argument_shadowing;
mod builtin_attribute_shadowing;
mod builtin_import_shadowing;
mod builtin_lambda_argument_shadowing;
mod builtin_module_shadowing;
mod builtin_variable_shadowing;
mod stdlib_module_shadowing;
Original file line number Diff line number Diff line change
Expand Up @@ -11,34 +11,35 @@ use crate::package::PackageRoot;
use crate::settings::types::PythonVersion;

/// ## What it does
/// Checks for modules that use the same names as Python builtin modules.
/// Checks for modules that use the same names as Python standard-library
/// modules.
///
/// ## Why is this bad?
/// Reusing a builtin module name for the name of a module increases the
/// difficulty of reading and maintaining the code, and can cause
/// non-obvious errors, as readers may mistake the variable for the
/// builtin and vice versa.
/// Reusing a standard-library module name for the name of a module increases
/// the difficulty of reading and maintaining the code, and can cause
/// non-obvious errors. Readers may mistake the first-party module for the
/// standard-library module and vice versa.
///
/// Builtin modules can be marked as exceptions to this rule via the
/// Standard-library modules can be marked as exceptions to this rule via the
/// [`lint.flake8-builtins.builtins-allowed-modules`] configuration option.
Comment on lines +23 to 24
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to update the settings name as well? (lint.flake8-builtins.builtins-allowed-modules) I think not but thought raise it regardless. Aside, the double "builtins" is a bit unfortunate.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, good catch. I think we should

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest to avoid the rename here and directly rename it to remove the "builtins" prefix from both options. I'll open a tracking issue but happy to hear any suggestions if you think otherwise.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

///
/// ## Options
/// - `lint.flake8-builtins.builtins-allowed-modules`
#[derive(ViolationMetadata)]
pub(crate) struct BuiltinModuleShadowing {
pub(crate) struct StdlibModuleShadowing {
name: String,
}

impl Violation for BuiltinModuleShadowing {
impl Violation for StdlibModuleShadowing {
#[derive_message_formats]
fn message(&self) -> String {
let BuiltinModuleShadowing { name } = self;
format!("Module `{name}` is shadowing a Python builtin module")
let StdlibModuleShadowing { name } = self;
format!("Module `{name}` shadows a Python standard-library module")
}
}

/// A005
pub(crate) fn builtin_module_shadowing(
pub(crate) fn stdlib_module_shadowing(
path: &Path,
package: Option<PackageRoot<'_>>,
allowed_modules: &[String],
Expand Down Expand Up @@ -74,7 +75,7 @@ pub(crate) fn builtin_module_shadowing(
}

Some(Diagnostic::new(
BuiltinModuleShadowing {
StdlibModuleShadowing {
name: module_name.to_string(),
},
TextRange::default(),
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
---
source: crates/ruff_linter/src/rules/flake8_builtins/mod.rs
snapshot_kind: text
---
__init__.py:1:1: A005 Module `logging` is shadowing a Python builtin module
__init__.py:1:1: A005 Module `logging` shadows a Python standard-library module
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
---
source: crates/ruff_linter/src/rules/flake8_builtins/mod.rs
snapshot_kind: text
---
bisect.py:1:1: A005 Module `bisect` is shadowing a Python builtin module
bisect.py:1:1: A005 Module `bisect` shadows a Python standard-library module
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
---
source: crates/ruff_linter/src/rules/flake8_builtins/mod.rs
snapshot_kind: text
---
bisect.py:1:1: A005 Module `bisect` is shadowing a Python builtin module
bisect.py:1:1: A005 Module `bisect` shadows a Python standard-library module
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
---
source: crates/ruff_linter/src/rules/flake8_builtins/mod.rs
snapshot_kind: text
---
xml.py:1:1: A005 Module `xml` is shadowing a Python builtin module
xml.py:1:1: A005 Module `xml` shadows a Python standard-library module
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
---
source: crates/ruff_linter/src/rules/flake8_builtins/mod.rs
snapshot_kind: text
---
__init__.py:1:1: A005 Module `string` is shadowing a Python builtin module
__init__.py:1:1: A005 Module `string` shadows a Python standard-library module
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
---
source: crates/ruff_linter/src/rules/flake8_builtins/mod.rs
snapshot_kind: text
---
__init__.py:1:1: A005 Module `string` is shadowing a Python builtin module
__init__.py:1:1: A005 Module `string` shadows a Python standard-library module
Loading