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

Avoid nested loops in missing_whitespace #3688

Merged
merged 1 commit into from
Mar 23, 2023
Merged

Avoid nested loops in missing_whitespace #3688

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
33 changes: 17 additions & 16 deletions crates/ruff/src/rules/pycodestyle/rules/missing_whitespace.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,14 @@
#![allow(dead_code, unused_imports, unused_variables)]

use itertools::Itertools;
use rustpython_parser::ast::Location;
use rustpython_parser::Tok;

use ruff_diagnostics::DiagnosticKind;
use ruff_diagnostics::Fix;
use ruff_diagnostics::Violation;
use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::types::Range;

use crate::registry::AsRule;
use crate::rules::pycodestyle::helpers::{is_keyword_token, is_singleton_token};

#[violation]
pub struct MissingWhitespace {
pub token: String,
Expand Down Expand Up @@ -40,21 +36,26 @@ pub fn missing_whitespace(
indent_level: usize,
) -> Vec<Diagnostic> {
let mut diagnostics = vec![];
for (idx, char) in line.chars().enumerate() {
if idx + 1 == line.len() {
break;

let mut num_lsqb = 0;
let mut num_rsqb = 0;
let mut prev_lsqb = None;
let mut prev_lbrace = None;
for (idx, (char, next_char)) in line.chars().tuple_windows().enumerate() {
if char == '[' {
Copy link
Member

Choose a reason for hiding this comment

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

Not. I like to use match statements if I have many branches matching on constant values. It gives you compile time evaluation that no teo branches are overlapping (and can be shorter than changing char == comparisons)

Copy link
Member Author

Choose a reason for hiding this comment

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

(I think a lot of this code is a line-by-line port of pycodestyle, for correctness purposes as we were trying to get the test suite passing. Doesn't mean it has to stay that way :))

num_lsqb += 1;
prev_lsqb = Some(idx);
} else if char == ']' {
num_rsqb += 1;
} else if char == '{' {
prev_lbrace = Some(idx);
}
let next_char = line.chars().nth(idx + 1).unwrap();

if ",;:".contains(char) && !char::is_whitespace(next_char) {
let before = &line[..idx];
if char == ':'
&& before.matches('[').count() > before.matches(']').count()
&& before.rfind('{') < before.rfind('[')
{
if (char == ',' || char == ';' || char == ':') && !char::is_whitespace(next_char) {
if char == ':' && num_lsqb > num_rsqb && prev_lsqb > prev_lbrace {
continue; // Slice syntax, no space required
}
if char == ',' && ")]".contains(next_char) {
if char == ',' && (next_char == ')' || next_char == ']') {
continue; // Allow tuple with only one element: (3,)
}
if char == ':' && next_char == '=' {
Expand Down