-
Notifications
You must be signed in to change notification settings - Fork 127
Package python-ldap with pyproject.toml #589
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
base: main
Are you sure you want to change the base?
Conversation
c64066f
to
4a48bc8
Compare
@hroncok hi! When/if you have time, could you please take a look if all is good with the Notably, it has C extensions that are handled in |
you added |
I would also suggest not mixing unrelated changes, but converting this to pyproject.toml as is and only then add more stuff (like ruff). |
I totally agree! It's better to be done in a separate PR. |
tox.ini
Outdated
[testenv:py313] | ||
# Python 3.13 headers are incompatible with declaration-after-statement | ||
setenv = | ||
CFLAGS=-Wno-int-in-bool-context -Werror -std=c99 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example this addition is not related.
[tool.coverage.run] | ||
branch = true | ||
source = [ | ||
"ldap", | ||
"ldif", | ||
"ldapurl", | ||
"slapdtest", | ||
] | ||
|
||
[tool.coverage.paths] | ||
source = [ | ||
"Lib/", | ||
".tox/*/lib/python*/site-packages/", | ||
] | ||
|
||
[tool.coverage.report] | ||
ignore_errors = false | ||
precision = 1 | ||
exclude_lines = [ | ||
"pragma: no cover", | ||
"raise NotImplementedError", | ||
"if 0:", | ||
"if __name__ == .__main__.:", | ||
"if PY2", | ||
"if not PY2", | ||
] | ||
|
||
[tool.coverage.html] | ||
directory = "build/htmlcov" | ||
title = "python-ldap coverage report" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not related and should be done in a separate commit at least.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's just a move of a coverage tool to pyproject.toml. I'd rather do it as part of this PR.
pyproject.toml
Outdated
[tool.black] | ||
line-length = 88 | ||
target-version = ['py36', 'py37', 'py38', 'py39', 'py310', 'py311', 'py312', 'py313'] | ||
extend-exclude = ''' | ||
/( | ||
\.eggs | ||
| \.git | ||
| \.hg | ||
| \.mypy_cache | ||
| \.tox | ||
| \.venv | ||
| _build | ||
| buck-out | ||
| build | ||
| dist | ||
| Modules | ||
)/ | ||
''' | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I'm fine doing it separately, too.
pyproject.toml
Outdated
line_length=88 | ||
known_first_party=['ldap', '_ldap', 'ldapurl', 'ldif', 'slapdtest'] | ||
sections=['FUTURE', 'STDLIB', 'THIRDPARTY', 'FIRSTPARTY', 'LOCALFOLDER'] | ||
profile = "black" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated.
pyproject.toml
Outdated
[tool.pytest.ini_options] | ||
testpaths = ["Tests"] | ||
python_files = ["t_*.py"] | ||
filterwarnings = [ | ||
"error", | ||
"ignore::ldap.LDAPBytesWarning", | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also seemingly unrelated.
I support the changes in general. As @hroncok pointed out, it's worth doing it one commit at a time (e.g. cleanup if needed, then switch to pyproject.toml as 1:1, then any changes to 3.13 if we really need to do them right now) The doc updates look quite useful, so personally I'd accept them as part of the cleanup set but others' opinions might differ. Adding |
No description provided.