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.

Sign up for GitHub

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

Jump to bottom

Support globbing in isort options #5473

Merged
merged 12 commits into from
Jul 7, 2023
Merged

Support globbing in isort options #5473

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
1810bb8
Support option
tjkuson Jul 2, 2023
a4c05de
Use glob patterns
tjkuson Jul 3, 2023
db904e5
Add glob matching logic
tjkuson Jul 3, 2023
fbdd5a5
Add test
tjkuson Jul 3, 2023
1b7e64a
Merge remote-tracking branch 'refs/remotes/origin/main' into glob
tjkuson Jul 3, 2023
836c4c8
Remove comment
tjkuson Jul 3, 2023
2216769
Merge remote-tracking branch 'refs/remotes/origin/main' into glob
tjkuson Jul 5, 2023
e9f79fd
Document glob behaviour
tjkuson Jul 5, 2023
d4747f3
Avoid some allocations
charliermarsh Jul 6, 2023
0ea5b6a
Merge branch 'main' into glob
charliermarsh Jul 6, 2023
235a914
Clean up some stuff
charliermarsh Jul 6, 2023
8febc8f
Tweaks
charliermarsh Jul 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
92 changes: 51 additions & 41 deletions crates/ruff/src/rules/isort/categorize.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
use std::collections::BTreeMap;
use std::hash::BuildHasherDefault;
use std::path::{Path, PathBuf};
use std::{fs, iter};

use log::debug;
use rustc_hash::FxHashMap;
use rustc_hash::{FxHashMap, FxHashSet};
use serde::{Deserialize, Serialize};
use strum_macros::EnumIter;

Expand Down Expand Up @@ -210,20 +211,20 @@ pub(crate) fn categorize_imports<'a>(
#[derive(Debug, Default, CacheKey)]
pub struct KnownModules {
/// A map of known modules to their section.
known: FxHashMap<String, ImportSection>,
known: Vec<(glob::Pattern, ImportSection)>,
/// Whether any of the known modules are submodules (e.g., `foo.bar`, as opposed to `foo`).
has_submodules: bool,
}

impl KnownModules {
pub(crate) fn new(
first_party: Vec<String>,
third_party: Vec<String>,
local_folder: Vec<String>,
standard_library: Vec<String>,
user_defined: FxHashMap<String, Vec<String>>,
first_party: Vec<glob::Pattern>,
third_party: Vec<glob::Pattern>,
local_folder: Vec<glob::Pattern>,
standard_library: Vec<glob::Pattern>,
user_defined: FxHashMap<String, Vec<glob::Pattern>>,
) -> Self {
let modules = user_defined
let known: Vec<(glob::Pattern, ImportSection)> = user_defined
.into_iter()
.flat_map(|(section, modules)| {
modules
Expand All @@ -249,19 +250,23 @@ impl KnownModules {
standard_library
.into_iter()
.map(|module| (module, ImportSection::Known(ImportType::StandardLibrary))),
);
)
.collect();

let mut known = FxHashMap::with_capacity_and_hasher(
modules.size_hint().0,
std::hash::BuildHasherDefault::default(),
);
modules.for_each(|(module, section)| {
if known.insert(module, section).is_some() {
warn_user_once!("One or more modules are part of multiple import sections.");
// Warn in the case of duplicate modules.
let mut seen =
FxHashSet::with_capacity_and_hasher(known.len(), BuildHasherDefault::default());
for (module, _) in &known {
if !seen.insert(module) {
warn_user_once!("One or more modules are part of multiple import sections, including: `{module}`");
break;
}
});
}

let has_submodules = known.keys().any(|module| module.contains('.'));
// Check if any of the known modules are submodules.
let has_submodules = known
.iter()
.any(|(module, _)| module.as_str().contains('.'));

Self {
known,
Expand Down Expand Up @@ -296,50 +301,55 @@ impl KnownModules {
}

fn categorize_submodule(&self, submodule: &str) -> Option<(&ImportSection, Reason)> {
if let Some(section) = self.known.get(submodule) {
let reason = match section {
ImportSection::UserDefined(_) => Reason::UserDefinedSection,
ImportSection::Known(ImportType::FirstParty) => Reason::KnownFirstParty,
ImportSection::Known(ImportType::ThirdParty) => Reason::KnownThirdParty,
ImportSection::Known(ImportType::LocalFolder) => Reason::KnownLocalFolder,
ImportSection::Known(ImportType::StandardLibrary) => Reason::ExtraStandardLibrary,
ImportSection::Known(ImportType::Future) => {
unreachable!("__future__ imports are not known")
}
};
Some((section, reason))
} else {
None
}
let section = self.known.iter().find_map(|(pattern, section)| {
if pattern.matches(submodule) {
Some(section)
} else {
None
}
})?;
let reason = match section {
ImportSection::UserDefined(_) => Reason::UserDefinedSection,
ImportSection::Known(ImportType::FirstParty) => Reason::KnownFirstParty,
ImportSection::Known(ImportType::ThirdParty) => Reason::KnownThirdParty,
ImportSection::Known(ImportType::LocalFolder) => Reason::KnownLocalFolder,
ImportSection::Known(ImportType::StandardLibrary) => Reason::ExtraStandardLibrary,
ImportSection::Known(ImportType::Future) => {
unreachable!("__future__ imports are not known")
}
};
Some((section, reason))
}

/// Return the list of modules that are known to be of a given type.
pub(crate) fn modules_for_known_type(&self, import_type: ImportType) -> Vec<String> {
pub(crate) fn modules_for_known_type(
&self,
import_type: ImportType,
) -> impl Iterator<Item = &glob::Pattern> {
self.known
.iter()
.filter_map(|(module, known_section)| {
.filter_map(move |(module, known_section)| {
if let ImportSection::Known(section) = known_section {
if *section == import_type {
Some(module.clone())
Some(module)
} else {
None
}
} else {
None
}
})
.collect()
}

/// Return the list of user-defined modules, indexed by section.
pub(crate) fn user_defined(&self) -> FxHashMap<String, Vec<String>> {
let mut user_defined: FxHashMap<String, Vec<String>> = FxHashMap::default();
pub(crate) fn user_defined(&self) -> FxHashMap<&str, Vec<&glob::Pattern>> {
let mut user_defined: FxHashMap<&str, Vec<&glob::Pattern>> = FxHashMap::default();
for (module, section) in &self.known {
if let ImportSection::UserDefined(section_name) = section {
user_defined
.entry(section_name.clone())
.entry(section_name.as_str())
.or_default()
.push(module.clone());
.push(module);
}
}
user_defined
Expand Down
44 changes: 36 additions & 8 deletions crates/ruff/src/rules/isort/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -369,6 +369,10 @@ mod tests {
Ok(())
}

fn pattern(pattern: &str) -> glob::Pattern {
glob::Pattern::new(pattern).unwrap()
}

#[test_case(Path::new("separate_subpackage_first_and_third_party_imports.py"))]
fn separate_modules(path: &Path) -> Result<()> {
let snapshot = format!("1_{}", path.to_string_lossy());
Expand All @@ -377,8 +381,32 @@ mod tests {
&Settings {
isort: super::settings::Settings {
known_modules: KnownModules::new(
vec!["foo.bar".to_string(), "baz".to_string()],
vec!["foo".to_string(), "__future__".to_string()],
vec![pattern("foo.bar"), pattern("baz")],
vec![pattern("foo"), pattern("__future__")],
vec![],
vec![],
FxHashMap::default(),
),
..super::settings::Settings::default()
},
src: vec![test_resource_path("fixtures/isort")],
..Settings::for_rule(Rule::UnsortedImports)
},
)?;
assert_messages!(snapshot, diagnostics);
Ok(())
}

#[test_case(Path::new("separate_subpackage_first_and_third_party_imports.py"))]
fn separate_modules_glob(path: &Path) -> Result<()> {
let snapshot = format!("glob_1_{}", path.to_string_lossy());
let diagnostics = test_path(
Path::new("isort").join(path).as_path(),
&Settings {
isort: super::settings::Settings {
known_modules: KnownModules::new(
vec![pattern("foo.*"), pattern("baz")],
vec![pattern("foo"), pattern("__future__")],
vec![],
vec![],
FxHashMap::default(),
Expand All @@ -401,8 +429,8 @@ mod tests {
&Settings {
isort: super::settings::Settings {
known_modules: KnownModules::new(
vec!["foo".to_string()],
vec!["foo.bar".to_string()],
vec![pattern("foo")],
vec![pattern("foo.bar")],
vec![],
vec![],
FxHashMap::default(),
Expand Down Expand Up @@ -445,7 +473,7 @@ mod tests {
known_modules: KnownModules::new(
vec![],
vec![],
vec!["ruff".to_string()],
vec![pattern("ruff")],
vec![],
FxHashMap::default(),
),
Expand Down Expand Up @@ -961,7 +989,7 @@ mod tests {
vec![],
vec![],
vec![],
FxHashMap::from_iter([("django".to_string(), vec!["django".to_string()])]),
FxHashMap::from_iter([("django".to_string(), vec![pattern("django")])]),
),
section_order: vec![
ImportSection::Known(ImportType::Future),
Expand Down Expand Up @@ -990,11 +1018,11 @@ mod tests {
src: vec![test_resource_path("fixtures/isort")],
isort: super::settings::Settings {
known_modules: KnownModules::new(
vec!["library".to_string()],
vec![pattern("library")],
vec![],
vec![],
vec![],
FxHashMap::from_iter([("django".to_string(), vec!["django".to_string()])]),
FxHashMap::from_iter([("django".to_string(), vec![pattern("django")])]),
),
section_order: vec![
ImportSection::Known(ImportType::Future),
Expand Down