Skip to content

enable E501 in ruff #4605

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

Closed
sir-sigurd wants to merge 1 commit into from
Closed

enable E501 in ruff #4605

Show file tree
Hide file tree
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
3 changes: 2 additions & 1 deletion py-shared/src/quilt_shared/athena.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,8 @@ def run_multiple_queries(
results: list[QueryExecutionTypeDef | None] = [None] * len(query_list)

remaining_queries = list(enumerate(query_list))
remaining_queries.reverse() # Just to make unit tests more sane: we use pop() later, so keep the order the same.
# Just to make unit tests more sane: we use pop() later, so keep the order the same.
remaining_queries.reverse()
pending_execution_ids = {}

while remaining_queries or pending_execution_ids:
Expand Down
1 change: 0 additions & 1 deletion ruff.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ select = [

# Ignore rules that were disabled in pylint or would cause too many changes
ignore = [
"E501", # line-too-long (already handled by line-length setting)

@nl0 nl0 Oct 15, 2025

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 don't think we should do that, because that applies two similar, but not exactly the same checks. we already handle that by the formatting task, so if anything has to be reformatted due to line length, it will cause an error in the formatting ci job

@sir-sigurd sir-sigurd Oct 15, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

i don't think we should do that, because that applies two similar, but not exactly the same checks.

I don't understand that

@nl0 nl0 Oct 15, 2025

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 mean ruff formatting logic is not that strict about the line lengths compared to the linting logic,
e.g. it doesn't complain on that comment line you changed in this PR, otherwise the formatting job would fail
oh i see we don't have a repo-wide formatting check in CI.
but anyways, i don't think we want two CI jobs checking one thing -- it's enough to enforce this in the formatting job (which is commented-out currently)

@nl0 nl0 Oct 15, 2025

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.

has ruff format done that change, btw?

@sir-sigurd sir-sigurd Oct 15, 2025 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

has ruff format done that change, btw?

no

but anyways, i don't think we want two CI jobs checking one thing

I don't see any problems with it and I don't want to have long lines with comments like I fixed in this PR (or with long string which AFAICT ruff won't reformat as well)

we have both black and pycodestyle in enterprise and I don't think it caused any issues
also prior to using ruff we had E501-like check in this repo

"E731", # lambda assignment
"B008", # function calls in argument defaults
"B904", # raise without from
Expand Down