Skip to content

Simplify arithmetic operation in logical lines checker#11346

Merged
charliermarsh merged 1 commit intoastral-sh:mainfrom
augustelalande:simplify-arithmetic
May 9, 2024
Merged

Simplify arithmetic operation in logical lines checker#11346
charliermarsh merged 1 commit intoastral-sh:mainfrom
augustelalande:simplify-arithmetic

Conversation

@augustelalande
Copy link
Copy Markdown
Contributor

Summary

Simplify arithmetic operation in logical lines checker

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented May 8, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Copy link
Copy Markdown
Member

@zanieb zanieb left a comment

Choose a reason for hiding this comment

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

Huh. It'd be good to get the eyes of the original author on this too.

@charliermarsh charliermarsh added the internal An internal refactor or improvement label May 9, 2024
@charliermarsh charliermarsh merged commit e9d1cdd into astral-sh:main May 9, 2024
@charliermarsh
Copy link
Copy Markdown
Member

Yeah that's interesting, hmm... Maybe it evolved over time? Maybe this was just the way they thought about the logic?

let tab_size = indent_width.as_usize();
for c in line.bytes() {
match c {
b'\t' => indent = (indent / tab_size) * tab_size + tab_size,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Wait sorry, is this the same? What if indent is 4 and tab_size is 3?

indent += tab_size would yield 7, but indent = (indent / tab_size) * tab_size + tab_size would yield 6, right?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I.e., is the goal here perhaps that indent ends up as a multiple of tab_size?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is the deal here that there's implicit integer rounding?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If so, can we add a comment explaining the deal? @charliermarsh

charliermarsh added a commit that referenced this pull request May 9, 2024
@augustelalande augustelalande deleted the simplify-arithmetic branch May 9, 2024 01:50
charliermarsh added a commit that referenced this pull request May 9, 2024
…" (#11348)

## Summary

I merged this, but I think it might not be the same behavior? See my
comment at:
#11346 (comment)
dhruvmanila pushed a commit that referenced this pull request May 17, 2024
## Summary

Simplify arithmetic operation in logical lines checker
dhruvmanila pushed a commit that referenced this pull request May 17, 2024
…" (#11348)

## Summary

I merged this, but I think it might not be the same behavior? See my
comment at:
#11346 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

internal An internal refactor or improvement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants