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

Fix E231 bug: Inconsistent catch compared to pycodestyle, such as when dict nested in list #10469

Merged
merged 4 commits into from
Mar 21, 2024

Conversation

augustelalande
Copy link
Contributor

Summary

Fix E231 bug: Inconsistent catch compared to pycodestyle, such as when dict nested in list. Resolves #10113.

Test Plan

Example from #10113 added to test fixture.

@augustelalande augustelalande force-pushed the e231-nested-bug branch 2 times, most recently from f446cd4 to 2837639 Compare March 19, 2024 04:55
Copy link
Contributor

github-actions bot commented Mar 19, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

@MichaReiser MichaReiser added the bug Something isn't working label Mar 19, 2024
@MichaReiser
Copy link
Member

Thanks for looking into this. Using a stack here makes sense. I'm surprised that this isn't hitting performance more, considering that we're now allocating a new Vec for each logical line (actually two).

@augustelalande
Copy link
Contributor Author

augustelalande commented Mar 20, 2024

Thanks for looking into this. Using a stack here makes sense. I'm surprised that this isn't hitting performance more, considering that we're now allocating a new Vec for each logical line (actually two).

Ya it's surprising. But is allocating a new Vec really significantly more expensive than allocating any other variable, if it's not populated?

@augustelalande
Copy link
Contributor Author

Took inspiration from extraneous_whitespace.rs and reduced it to one stack

@MichaReiser
Copy link
Member

MichaReiser commented Mar 21, 2024

Ya it's surprising. But is allocating a new Vec really significantly more expensive than allocating any other variable, if it's not populated?

That's correct: Creating an empty Vec is "free" because it doesn't allocate. But a Vec is different from e.g. a TextSize in that it allocates memory on the heap when you push the first element. That's different from a TextSize variable that is always stored on the stack (which is as cheap as memory can be)

Took inspiration from extraneous_whitespace.rs and reduced it to one stack

Nice, thank you

@@ -97,7 +97,7 @@ pub(crate) fn missing_whitespace(line: &LogicalLine, context: &mut LogicalLinesC
if let Some(next_token) = iter.peek() {
match (kind, next_token.kind()) {
(TokenKind::Colon, _)
if open_parentheses > 0 && prev_lsqb > prev_lbrace =>
if matches!(brackets.last(), Some(TokenKind::Lsqb)) =>
Copy link
Member

@MichaReiser MichaReiser Mar 21, 2024

Choose a reason for hiding this comment

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

I find this much easier to understand. Nice refactor

@MichaReiser MichaReiser merged commit d9ac170 into astral-sh:main Mar 21, 2024
17 checks passed
@augustelalande augustelalande deleted the e231-nested-bug branch March 21, 2024 12:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[E231] Inconsistent catch compared to pycodestyle, such as when dict nested in list
2 participants