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

Ruff linter: Unexcluded _sync folders, Used default line-length #887

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

T-256
Copy link
Contributor

@T-256 T-256 commented Feb 13, 2024

Summary

Same as what's done on httpx: encode/httpx#2922

Checklist

  • I understand that this PR may be closed in case there was no previous discussion. (This doesn't apply to typos!)
  • I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
  • I've updated the documentation accordingly.

pyproject.toml Outdated Show resolved Hide resolved
Comment on lines +117 to +121

[tool.ruff.lint.per-file-ignores]
# needed for unasync.py
"httpcore/_sync/*" = ["I001"]
"tests/_sync/*" = ["I001"]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If remove this section, result will be:

>ruff check
httpcore\_sync\connection.py:1:1: I001 [*] Import block is un-sorted or un-formatted
httpcore\_sync\connection_pool.py:1:1: I001 [*] Import block is un-sorted or un-formatted
httpcore\_sync\socks_proxy.py:1:1: I001 [*] Import block is un-sorted or un-formatted
tests\_sync\test_connection.py:1:1: I001 [*] Import block is un-sorted or un-formatted
tests\_sync\test_connection_pool.py:1:1: I001 [*] Import block is un-sorted or un-formatted
Found 5 errors.
[*] 5 fixable with the `--fix` option.

Maybe we also could use in-file rule ignore for them?

Copy link
Member

Choose a reason for hiding this comment

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

Do we really need to avoid excluding sync directories? When we need to format/lint them? they are auto generated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are pre-generated. it means currently, they are already available in sdist, wheels and repository.
It doesn't look good that readers navigate through unformatted sources after installation.
For example, after installation, I'll navigate to site-packages/httpcore then in _sync folder's contents I open a single file, then I'll see linter/formatter warnings in my IDE.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When it's possible to have them, why excluding them?

Copy link
Member

@karpetrosyan karpetrosyan Apr 15, 2024

Choose a reason for hiding this comment

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

I'll navigate to site-packages/httpcore then in _sync folder's contents I open a single file, then I'll see linter/formatter warnings in my IDE.

I believe it is typical to see such situations because IDE rules and rules used for linting source files are not always in sync.

When it's possible to have them, why excluding them?

We can, for the time being, ignore I001 for those files, but we don't know how many additional rules may be broken when introducing a new feature. We need add new rules that break sync files into the configuration every time, which makes the maintenance process more difficult.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we don't know how many additional rules may be broken when introducing a new feature. We need add new rules that break sync files into the configuration every time, which makes the maintenance process more difficult.

The codebase is almost stable, I think the only violation rule would be import sorting violation. by checking unasync.py script I don't except new violation happening when adding new feature other than I001 (and F811 which can be in-line ignoring this without changing pyproject.toml)

IMO, we could give it a try, since always doable in future to revert this change.

unasync.py Outdated Show resolved Hide resolved
@T-256 T-256 mentioned this pull request Apr 13, 2024
@T-256 T-256 changed the title Ruff linter: Use the default line-length Ruff linter: Unexcluded _sync folders, Used default line-length Apr 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants