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

feat: Support configuration in .cython-lint.toml #104

Merged
merged 1 commit into from
Apr 15, 2024

Conversation

dalcinl
Copy link
Contributor

@dalcinl dalcinl commented Apr 15, 2024

Fixes #84.

cc @webknjaz

Copy link
Owner

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

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

thanks @dalcinl

@MarcoGorelli MarcoGorelli merged commit b899ac6 into MarcoGorelli:main Apr 15, 2024
11 checks passed
@dalcinl dalcinl deleted the alt-config branch April 15, 2024 09:20
@dalcinl
Copy link
Contributor Author

dalcinl commented Apr 15, 2024

@MarcoGorelli Any estimate for an upcoming release with this feature?

@MarcoGorelli
Copy link
Owner

hey @dalcinl , I just pushed one out, does it work for you?

@dalcinl
Copy link
Contributor Author

dalcinl commented Apr 15, 2024

Yes, it works perfectly!

There is a minor nit, though, unrelated to this PR.

Calling cython-lint without arguments fail with traceback

$ cython-lint
Traceback (most recent call last):
  File "/home/dalcinl/.local/bin/cython-lint", line 8, in <module>
    sys.exit(main())
             ^^^^^^
  File "/home/dalcinl/.local/lib/python3.12/site-packages/cython_lint/cython_lint.py", line 930, in main
    config = {k.replace("-", "_"): v for k, v in _get_config(paths).items()}
                                                 ^^^^^^^^^^^^^^^^^^
  File "/home/dalcinl/.local/lib/python3.12/site-packages/cython_lint/cython_lint.py", line 880, in _get_config
    root = pathlib.Path(os.path.commonpath(paths))
                        ^^^^^^^^^^^^^^^^^^^^^^^^^
  File "<frozen posixpath>", line 550, in commonpath
ValueError: commonpath() arg is an empty sequence

This should either print usage and exit with error code, or maybe better make it equivalent to cython-lint ., that is, lint recursively from the current working directory. Let me know your preference and I'll submit a PR, together with support for running with python -m cython_lint <args>.

@MarcoGorelli
Copy link
Owner

print usage and exit with error code

Sounds good

@dalcinl
Copy link
Contributor Author

dalcinl commented Apr 15, 2024

lint recursively from the current working directory

So, don't you like this other alternative? It is the behavior of ruff check and flake8. But you have no attachment to behavioral consistency with flake8, that's fine with me. I'm just trying to point out the rationale for the alternative.

@@ -768,6 +768,17 @@ def test_config_file(tmpdir: Any, capsys: Any) -> None:
for file in cython_files:
assert f'{file}:1:11: E701 multiple statements on one line' in out

# .cython-lint.toml takes precedence, even if empty
config_file = os.path.join(tmpdir, '.cython-lint.toml')
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: it's best to write separate test functions for separate cases. Otherwise, the test report is messy and parts of a test function may not even run when there's problems.

# .cython-lint.toml takes precedence over
# pyproject.toml even when it is empty
if basename == '.cython-lint.toml':
return config
Copy link
Contributor

Choose a reason for hiding this comment

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

I have a feeling that the tests don't cover this case so it can break suddenly..

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.

[FR] Support a standalone config file .cython-lint.yml
3 participants