Skip to content

Commit

Permalink
fix(linter): fix file name checking behavior of `unicorn/filename-cas…
Browse files Browse the repository at this point in the history 
…e` (#6463)

- closes #6459

Currently, the `unicorn/filename-case` doesn't match the behavior of the original rule. There are several issues which this PR fixes:

- The defaults cases were incorrect: only kebab case should be enabled by default, according to the original rule docs.
- Setting a single case or multiple cases did not remove the default cases as it should.
- Leading/trailing underscores were not ignored by default.
- We did not provide a clear diagnostic message indicating which cases are allowed.
- We did not try to parse out multiple file parts (separated by `.`).
  - TODO: We should also support multiple file part checking (which the original rule supports via a config option), for file names such as `someTest.fileName.js`

I have also added many of the original test cases to ensure we are more closely compatible.

This also improves the performance of just running the `unicorn/filename-case` alone by 4% (20ms total) on the `vscode` codebase:

```
Benchmark 1: ./oxlint-main -A all -W unicorn/filename-case --silent vscode
  Time (mean ± σ):     489.4 ms ±  24.0 ms    [User: 2623.8 ms, System: 447.2 ms]
  Range (min … max):   474.7 ms … 622.9 ms    100 runs

  Warning: Statistical outliers were detected. Consider re-running this benchmark on a quiet system without any interferences from other programs. It might help to use the '--warmup' or '--prepare' options.

Benchmark 2: ./oxlint-new-filename-case -A all -W unicorn/filename-case --silent vscode
  Time (mean ± σ):     470.8 ms ±  22.9 ms    [User: 2478.8 ms, System: 463.5 ms]
  Range (min … max):   455.6 ms … 599.3 ms    100 runs

  Warning: Statistical outliers were detected. Consider re-running this benchmark on a quiet system without any interferences from other programs. It might help to use the '--warmup' or '--prepare' options.

Summary
  ./oxlint-new-filename-case -A all -W unicorn/filename-case --silent vscode ran
    1.04 ± 0.07 times faster than ./oxlint-main -A all -W unicorn/filename-case --silent vscode
```

Perplexingly, it seems like it might actually be even faster on the `vscode` repository, saving ~5% (70ms) with the default ruleset enabled as well. Maybe my laptop was just running a bit faster.

```
Benchmark 1: ./oxlint-main -W unicorn/filename-case --silent vscode
  Time (mean ± σ):      1.402 s ±  0.096 s    [User: 8.863 s, System: 0.505 s]
  Range (min … max):    1.318 s …  1.920 s    100 runs

  Warning: Statistical outliers were detected. Consider re-running this benchmark on a quiet system without any interferences from other programs. It might help to use the '--warmup' or '--prepare' options.

Benchmark 2: ./oxlint-new-filename-case -W unicorn/filename-case --silent vscode
  Time (mean ± σ):      1.339 s ±  0.042 s    [User: 8.582 s, System: 0.511 s]
  Range (min … max):    1.266 s …  1.506 s    100 runs

Summary
  ./oxlint-new-filename-case -W unicorn/filename-case --silent vscode ran
    1.05 ± 0.08 times faster than ./oxlint-main -W unicorn/filename-case --silent vscode
```
  • Loading branch information
@camchenry
camchenry committed Oct 13, 2024
1 parent 725f9f6 commit d6a0d2e
Show file tree
Hide file tree
Showing 2 changed files with 311 additions and 63 deletions.
262 changes: 205 additions & 57 deletions crates/oxc_linter/src/rules/unicorn/filename_case.rs
View file
Original file line number Diff line number Diff line change
@@ -1,46 +1,86 @@
use convert_case::{Case, Casing};
use convert_case::{Boundary, Case, Converter};
use oxc_diagnostics::OxcDiagnostic;
use oxc_macros::declare_oxc_lint;
use oxc_span::Span;
use serde_json::Value;

use crate::{context::LintContext, rule::Rule};

fn filename_case_diagnostic(span: Span, case_name: &str) -> OxcDiagnostic {
OxcDiagnostic::warn(format!("Filename should not be in {case_name} case")).with_label(span)
fn join_strings_disjunction(strings: &[String]) -> String {
let mut list = String::new();
for (i, s) in strings.iter().enumerate() {
if i == 0 {
list.push_str(s);
} else if i == strings.len() - 1 {
list.push_str(&format!(", or {s}"));
} else {
list.push_str(&format!(", {s}"));
}
}
list
}

fn filename_case_diagnostic(valid_cases: &[&str]) -> OxcDiagnostic {
let case_names = valid_cases.iter().map(|s| format!("{s} case")).collect::<Vec<_>>();
let message = format!("Filename should be in {}", join_strings_disjunction(&case_names));
OxcDiagnostic::warn(message).with_label(Span::default())
}

#[derive(Debug, Clone)]
#[allow(clippy::struct_field_names)]
pub struct FilenameCase {
/// Whether kebab case is allowed.
kebab_case: bool,
/// Whether camel case is allowed.
camel_case: bool,
/// Whether snake case is allowed.
snake_case: bool,
/// Whether pascal case is allowed.
pascal_case: bool,
underscore_case: bool,
}

impl Default for FilenameCase {
fn default() -> Self {
Self {
kebab_case: false,
camel_case: true,
snake_case: false,
pascal_case: true,
underscore_case: false,
}
Self { kebab_case: true, camel_case: false, snake_case: false, pascal_case: false }
}
}

declare_oxc_lint!(
/// ### What it does
///
/// Enforce a case style for filenames.
/// Enforces specific case styles for filenames. By default, kebab case is enforced.
///
/// ### Why is this bad?
///
/// ### Example
/// ```
/// Inconsistent file naming conventions can make it harder to locate files or to create new ones.
///
/// ### Cases
///
/// Examples of **correct** filenames for each case:
///
/// #### `kebabCase`
///
/// - `some-file-name.js`
/// - `some-file-name.test.js`
/// - `some-file-name.test-utils.js`
///
/// #### `camelCase`
///
/// - `someFileName.js`
/// - `someFileName.test.js`
/// - `someFileName.testUtils.js`
///
/// #### `snakeCase`
///
/// - `some_file_name.js`
/// - `some_file_name.test.js`
/// - `some_file_name.test_utils.js`
///
/// #### `pascalCase`
///
/// - `SomeFileName.js`
/// - `SomeFileName.Test.js`
/// - `SomeFileName.TestUtils.js`
FilenameCase,
style
);
Expand All @@ -51,37 +91,27 @@ impl Rule for FilenameCase {
return Self::default();
};

if let Some(Value::String(s)) = case_type.get("case") {
return match s.as_str() {
"kebabCase" => Self { kebab_case: true, ..Self::default() },
"camelCase" => Self { camel_case: true, ..Self::default() },
"snakeCase" => Self { snake_case: true, ..Self::default() },
"pascalCase" => Self { pascal_case: true, ..Self::default() },
"underscoreCase" => Self { underscore_case: true, ..Self::default() },
_ => Self::default(),
};
}
let off =
Self { kebab_case: false, camel_case: false, snake_case: false, pascal_case: false };

if let Some(Value::String(s)) = case_type.get("case") {
return match s.as_str() {
"kebabCase" => Self { kebab_case: true, ..Self::default() },
"camelCase" => Self { camel_case: true, ..Self::default() },
"snakeCase" => Self { snake_case: true, ..Self::default() },
"pascalCase" => Self { pascal_case: true, ..Self::default() },
"underscoreCase" => Self { underscore_case: true, ..Self::default() },
"kebabCase" => Self { kebab_case: true, ..off },
"camelCase" => Self { camel_case: true, ..off },
"snakeCase" => Self { snake_case: true, ..off },
"pascalCase" => Self { pascal_case: true, ..off },
_ => Self::default(),
};
}

if let Some(Value::Object(map)) = case_type.get("cases") {
let mut filename_case = Self::default();
let mut filename_case = off;
for (key, value) in map {
match (key.as_str(), value) {
("kebabCase", Value::Bool(b)) => filename_case.kebab_case = *b,
("camelCase", Value::Bool(b)) => filename_case.camel_case = *b,
("snakeCase", Value::Bool(b)) => filename_case.snake_case = *b,
("pascalCase", Value::Bool(b)) => filename_case.pascal_case = *b,
("underscoreCase", Value::Bool(b)) => filename_case.underscore_case = *b,
_ => (),
}
}
Expand All @@ -96,27 +126,29 @@ impl Rule for FilenameCase {
return;
};

let mut case_name = None;
// get filename up to the first dot, or the whole filename if there is no dot
let filename = filename.split('.').next().unwrap_or(filename);
// ignore all leading and trailing underscores
let filename = filename.trim_matches('_');

let cases = [
(Case::Kebab, "kebab", self.kebab_case),
(Case::Camel, "camel", self.camel_case),
(Case::Snake, "snake", self.snake_case),
(Case::Pascal, "pascal", self.pascal_case),
(Case::Pascal, "underscore", self.underscore_case),
(self.camel_case, Case::Camel, "camel"),
(self.kebab_case, Case::Kebab, "kebab"),
(self.snake_case, Case::Snake, "snake"),
(self.pascal_case, Case::Pascal, "pascal"),
];
let mut enabled_cases = cases.iter().filter(|(enabled, _, _)| *enabled);

for (case, name, condition) in cases {
if filename.to_case(case) == filename {
if condition {
return;
}
case_name.replace(name);
}
}

if let Some(case_name) = case_name {
ctx.diagnostic(filename_case_diagnostic(Span::default(), case_name));
if !enabled_cases.any(|(_, case, _)| {
let converter =
Converter::new().remove_boundaries(&[Boundary::LowerDigit, Boundary::DigitLower]);
converter.to_case(*case).convert(filename) == filename
}) {
let case_names = cases
.iter()
.filter_map(|(enabled, _, name)| if *enabled { Some(*name) } else { None })
.collect::<Vec<_>>();
ctx.diagnostic(filename_case_diagnostic(&case_names));
}
}
}
Expand All @@ -127,18 +159,134 @@ fn test() {

use crate::tester::Tester;

fn test_case(
path: &'static str,
casing: &'static str,
) -> (&'static str, Option<Value>, Option<Value>, Option<PathBuf>) {
(
"",
if casing.is_empty() {
None
} else {
Some(serde_json::json!(
[{ "case": casing }]
))
},
None,
Some(PathBuf::from(path)),
)
}

fn test_cases<const N: usize>(
path: &'static str,
casings: [&'static str; N],
) -> (&'static str, Option<Value>, Option<Value>, Option<PathBuf>) {
(
"",
if casings.is_empty() {
None
} else {
let mut map = serde_json::Map::new();
// turn ["camelCase", "snakeCase"] into [{ "cases": { "camelCase": true, "snakeCase": true } }]
for casing in casings {
map.insert(casing.to_string(), Value::Bool(true));
}
Some(serde_json::json!([{ "cases": map }]))
},
None,
Some(PathBuf::from(path)),
)
}

let pass = vec![
// should pass - camel_case, pascal_case both allowed
("", None, None, Some(PathBuf::from("foo/bar/baz/Que.tsx"))),
// should pass - camel_case, pascal_case both allowed
("", None, None, Some(PathBuf::from("foo/bar/baz/QueAbc.tsx"))),
("", None, None, Some(PathBuf::from("ansiHTML.tsx"))),
test_cases("src/foo/fooBar.js", ["camelCase"]),
test_case("src/foo/bar.js", "camelCase"),
test_case("src/foo/fooBar.js", "camelCase"),
test_case("src/foo/bar.test.js", "camelCase"),
test_case("src/foo/fooBar.test.js", "camelCase"),
test_case("src/foo/fooBar.test-utils.js", "camelCase"),
test_case("src/foo/fooBar.test_utils.js", "camelCase"),
test_case("src/foo/.test_utils.js", "camelCase"),
test_case("src/foo/foo.js", "snakeCase"),
test_case("src/foo/foo_bar.js", "snakeCase"),
test_case("src/foo/foo.test.js", "snakeCase"),
test_case("src/foo/foo_bar.test.js", "snakeCase"),
test_case("src/foo/foo_bar.test_utils.js", "snakeCase"),
test_case("src/foo/foo_bar.test-utils.js", "snakeCase"),
test_case("src/foo/.test-utils.js", "snakeCase"),
test_case("src/foo/foo.js", "kebabCase"),
test_case("src/foo/foo-bar.js", "kebabCase"),
test_case("src/foo/foo.test.js", "kebabCase"),
test_case("src/foo/foo-bar.test.js", "kebabCase"),
test_case("src/foo/foo-bar.test-utils.js", "kebabCase"),
test_case("src/foo/foo-bar.test_utils.js", "kebabCase"),
test_case("src/foo/.test_utils.js", "kebabCase"),
test_case("src/foo/Foo.js", "pascalCase"),
test_case("src/foo/FooBar.js", "pascalCase"),
test_case("src/foo/Foo.test.js", "pascalCase"),
test_case("src/foo/FooBar.test.js", "pascalCase"),
test_case("src/foo/FooBar.test-utils.js", "pascalCase"),
test_case("src/foo/FooBar.test_utils.js", "pascalCase"),
test_case("src/foo/.test_utils.js", "pascalCase"),
test_case("spec/iss47Spec.js", "camelCase"),
test_case("spec/iss47Spec100.js", "camelCase"),
test_case("spec/i18n.js", "camelCase"),
test_case("spec/iss47-spec.js", "kebabCase"),
test_case("spec/iss-47-spec.js", "kebabCase"),
test_case("spec/iss47-100spec.js", "kebabCase"),
test_case("spec/i18n.js", "kebabCase"),
test_case("spec/iss47_spec.js", "snakeCase"),
test_case("spec/iss_47_spec.js", "snakeCase"),
test_case("spec/iss47_100spec.js", "snakeCase"),
test_case("spec/i18n.js", "snakeCase"),
test_case("spec/Iss47Spec.js", "pascalCase"),
test_case("spec/Iss47.100spec.js", "pascalCase"),
test_case("spec/I18n.js", "pascalCase"),
test_case("src/foo/_fooBar.js", "camelCase"),
test_case("src/foo/___fooBar.js", "camelCase"),
test_case("src/foo/_foo_bar.js", "snakeCase"),
test_case("src/foo/___foo_bar.js", "snakeCase"),
test_case("src/foo/_foo-bar.js", "kebabCase"),
test_case("src/foo/___foo-bar.js", "kebabCase"),
test_case("src/foo/_FooBar.js", "pascalCase"),
test_case("src/foo/___FooBar.js", "pascalCase"),
test_case("src/foo/$foo.js", "pascalCase"),
test_cases("src/foo/foo-bar.js", []),
test_cases("src/foo/fooBar.js", ["camelCase"]),
test_cases("src/foo/FooBar.js", ["kebabCase", "pascalCase"]),
test_cases("src/foo/___foo_bar.js", ["snakeCase", "pascalCase"]),
];
let fail = vec![
// should pass - by default kebab_case is not allowed
("import foo from 'bar'", None, None, Some(PathBuf::from("foo/bar/baz/aaa-bbb.tsx"))),
// should pass - by default snake_case is not allowed
("baz;", None, None, Some(PathBuf::from("foo/bar/baz/foo_bar.tsx"))),
test_case("src/foo/foo_bar.js", ""),
// todo: linter does not support uppercase JS files
// test_case("src/foo/foo_bar.JS", "camelCase"),
test_case("src/foo/foo_bar.test.js", "camelCase"),
test_case("test/foo/foo_bar.test_utils.js", "camelCase"),
test_case("test/foo/fooBar.js", "snakeCase"),
test_case("test/foo/fooBar.test.js", "snakeCase"),
test_case("test/foo/fooBar.testUtils.js", "snakeCase"),
test_case("test/foo/fooBar.js", "kebabCase"),
test_case("test/foo/fooBar.test.js", "kebabCase"),
test_case("test/foo/fooBar.testUtils.js", "kebabCase"),
test_case("test/foo/fooBar.js", "pascalCase"),
test_case("test/foo/foo_bar.test.js", "pascalCase"),
test_case("test/foo/foo-bar.test-utils.js", "pascalCase"),
test_case("src/foo/_FOO-BAR.js", "camelCase"),
test_case("src/foo/___FOO-BAR.js", "camelCase"),
test_case("src/foo/_FOO-BAR.js", "snakeCase"),
test_case("src/foo/___FOO-BAR.js", "snakeCase"),
test_case("src/foo/_FOO-BAR.js", "kebabCase"),
test_case("src/foo/___FOO-BAR.js", "kebabCase"),
test_case("src/foo/_FOO-BAR.js", "pascalCase"),
test_case("src/foo/___FOO-BAR.js", "pascalCase"),
test_cases("src/foo/foo_bar.js", []),
test_cases("src/foo/foo-bar.js", ["camelCase", "pascalCase"]),
test_cases("src/foo/_foo_bar.js", ["camelCase", "pascalCase", "kebabCase"]),
test_cases("src/foo/_FOO-BAR.js", ["snakeCase"]),
test_case("src/foo/[foo_bar].js", ""),
test_case("src/foo/$foo_bar.js", ""),
test_case("src/foo/$fooBar.js", ""),
test_cases("src/foo/{foo_bar}.js", ["camelCase", "pascalCase", "kebabCase"]),
];

Tester::new(FilenameCase::NAME, pass, fail).test_and_snapshot();
Expand Down
Loading

0 comments on commit d6a0d2e

Please sign in to comment.