[python-ldap] Add type hints#522
[python-ldap] Add type hints#522Alphix wants to merge 35 commits into
Conversation
|
With regard to the CI failures, I should add that With this patch applied: |
|
Hi, thanks for the PR! Looks good, would you be able to split it into a few more commits each doing self-contained changes: any fixes, And just out of curiosity, is there a reason you chose to use |
There was a problem hiding this comment.
Excellent work! A few comments and suggestions to push your PR over the finish line:
- Fix Python 3.6 compatibility issues. We still support old Python versions for $REASONS..., sorry.
- Please don't introduce yet another top-level module name. We like to move
ldapurlandldifinto theldappackage eventually. I suggestldap.typesmodule. - Don't use
from ldap_types import *. Star-imports are frowned upon and make code harder to read. - The PR is missing a PEP 561 type marker file. Add a
Lib/ldap/py.typedfile and add the file toMANIFEST.in - Move
mypy.inicontent to eitherpyproject.tomlorsetup.cfg - Extend
tox.iniwith a mypy check for every supported Python version - Add
_ldap.pyitoMANIFEST.in - Would it make sense to move
_ldap.pyitoLib/_ldap.pyiand install it along the shared extension?
Python 3.6 fails because it does not support future annotations:
File "/home/runner/work/python-ldap/python-ldap/.tox/c90-py36/lib/python3.6/site-packages/ldap/__init__.py", line 6
from __future__ import annotations
^
SyntaxError: future feature annotations is not defined
|
Hi! Why has this this work stalled? @Alphix are you still interested in continuing this?
What are those reasons? Many projects have already dropped 3.7. |
@intgr - it's the usual reason, I need to find the time to take the comments into account and update the patch. Maybe next week, we'll see...
My problem is that |
Because we (downstream vendor) still support Python 3.6. |
My question was: why can't python-ldap drop support for Python 3.6 (and maybe 3.7)? |
My guess would be RHEL? Anyway, does |
|
41ffdf3 to
8b4736f
Compare
That was an oversight, didn't realise it was relevant for backwards compatibility, I've fixed that in the second version. |
@tiran : I think I've addressed all the points you raised (and the CI seems happy). The only thing I can think of is that Also, I should add that this is just a starting point for type annotations. There are still a bunch of uses of |
Scratch that, I am working on splitting the PR into several preparatory patches, one big type hints patch, and one final patch integrating type checking (and learning a lot about
|
8b4736f to
e203e04
Compare
Ok, done. I've split the patch into a bunch of commits doing a lot of prep work, then one big patch adding the type hints only and finally one patch integrating |
intgr
left a comment
There was a problem hiding this comment.
Awesome, thanks for your work. I tried it out in my project and have some feedback.
e203e04 to
ba337e8
Compare
|
Ok, I've pushed a new version taking @intgr's comments into account. Most controversial change here is the move of |
|
@tiran gentle ping? |
|
Hey, @Alphix, can you update this to resolve conflicts, and then page @droideck, @mistotebe, and @tiran for review? Thanks so much... this is a tremendous contribution that shouldn't be left to bit-rot. |
@macserv Eh, I'm a volunteer, using my free time for this. If you want to push this forward, I'd suggest you try to get in touch with the maintainers...once there's some sign of interest from their side, I'd be happy to rebase and resolve conflicts... |
|
@Alphix Sorry for the late reply! Maintainer is here! If you are still around, could you please rebase it? |
Why is this necessary?
That upstream issue is still open. |
ba337e8 to
2726abc
Compare
Not sure I understand the question? Is the commit message unclear? |
decodeControlValue() expects a binary encodedControlValue, so correct the comparison to reflect that.
The comments for the ProxyAuthzControl and AuthorizationIdentityResponseControl classes indicate that they expect/return a str (which makes sense), but then fail to actually deal with the str properly. The GetEffectiveRightsControl class has a similar issue (but seems to be a stub at the moment, the "authzId" parameter should probably be renamed "gerSubject" and can take much more than just a DN).
Avoid redefining a variable to a different type.
This helps to avoid import loops in later patches.
This helps with the type checking later.
Make class ResultProcessor an explicit subclass of LDAPObject This keeps type checkers happy but shouldn't have any functional difference.
Avoid redefining variables to keep type checkers happy.
First, the criticality in class SyncRequestControl can be passed as an int/bool/etc, but it still makes sense (and helps type checkers) to make sure that it is actually stored as a bool. Second, some minor code changes to exclude the possibility that some variables are not None and to help type checkers understand the type of some objects. Last, make sure that the syncrepl_get_cookie() function is type-conformant.
__eq__ methods need to be able to handle being passed any kind of object. The remaining changes mostly serve to make it clearer to type checkers if/when a variable can/cannot be None.
Mostly some explicit None checks, avoiding variable redefinition and the removal of a circuitous import.
Mostly some code refactoring to avoid variable redefinition and to add some additional None checks. Make sure that _unparseChangeRecord can handle "bytes | List[bytes]" (since that will be part of the type definition of 3-tuple modifications). Also, change "valid_changetype_dict" to "valid_changetype_set" (since the variable is anyway used as a set, not as a dict).
Mostly some small changes to avoid variable redefinition and changing some functions to remove superfluous return values.
Instead of the magic loop in subentry.py which looks for appropriate classes from Lib/ldap/schema/models.py, let the classes register themselves explicitly in the SCHEMA_CLASS_MAPPING/SCHEMA_ATTR_MAPPING in order to not confuse type checkers (this should also be clearer to humans reading the code). In addition, add some more comments to subentry.py and do the usual type safety fixups (explicit type checks, asserts, avoiding variable redefinitions, etc).
First, rewrite extract_tokens() in Lib/ldap/schema/tokenizer.py as parse_tokens() and document the function. Then, use the new function in Lib/ldap/schema/models.py, and remove the token_defaults class attributes (which confuse type checkers as they can't decide which attributes a given class does/doesn't have) and instead set the attribute defaults explicitly. This might look like a big change, but most of it is repetitive changes throughout the classes in Lib/ldap/schema/models.py.
Small fix to keep type checkers happy.
Essentially, make sure that the cookie is always stored as binary
self.responseValue is bytes, not a str
…ntrols/* Several of the request controls are registered in KNOWN_RESPONSE_CONTROLS, which appears to be due to a bit too much copy-pasting.
Phew, after all the prep work, the actual type annotations can finally be added.
This is necessary in order for type checking due to: python/typing#1333 Given that _ldap is an internal module, this change is hopefully ok.
This helps in ensuring that the stub and the C module don't drift apart.
- Rebase type-annotations work onto current main (resolving conflicts) - Drop Python 3.6-3.8 support: require Python >= 3.9 in pyproject.toml, update typing imports accordingly (remove typing_extensions shims) - Fix mypy --strict failures introduced by updated pyasn1 stubs - Fix AnyStr usage in syncrepl.py (replace with Union[str, bytes]) - Fix Dict key type in OpenLDAPSyncreplCookie._csnset (was Dict[int,str]) - Add class-level `responseName: Optional[str]` to ExtendedResponse so the attribute is accessible on the class object in ldapobject.py - Add class-level `SCHEMADIR: Optional[str]` annotation in _slapdtest.py - Make uri parameter Optional[str] in SimpleLDAPObject and ReconnectLDAPObject to match main's default-None usage - Add types-pyasn1 to tox mypy environment deps Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
threading has been a mandatory module since Python 3.7 (PEP 528).
Python 3.10 introduced the X | Y union syntax (PEP 604) and Optional[X] is now considered legacy style. Since the project requires Python >= 3.9, add 'from __future__ import annotations' (PEP 563) to every affected module so the new syntax is valid at runtime on 3.9 as well.
constants.py is already the canonical source for the constants exposed
by the C extension (it drives constants_generated.h). Add a
generate_pyi() function alongside the existing print_header(), so that
the constants and error-class sections of _ldap.pyi can be regenerated
whenever the native interface changes:
python Lib/ldap/constants.py --pyi
Older versions of types-pyasn1 define SequenceOfAndSetOfBase.componentType as Optional[NamedTypes], causing mypy to reject assignments of concrete ASN.1 types in SequenceOf/SetOf subclasses. Newer mypy/types-pyasn1 (e.g. mypy 1.15) fix the stubs and consider such type:ignore comments unused. Add # type: ignore[assignment] to the five affected assignments, and set warn_unused_ignores = False per-module in setup.cfg so newer mypy does not reject the comments that older CI mypy still needs. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@mistotebe Ok, I've pushed an updated version. I hope I understood what you meant with "generate the |
With this patch applied,
MYPYPATH=/Stubs mypy --strict Lib/passes without any warnings.toxalso passes for everything frompy37topy11(and also includingdocandpypy3).I've tried my best to not make more modifications than strictly necessary. Most of the time, changes merely serve to make life easier for
mypy(i.e. to enable type inference) or to fixup non-type aware code.The patch might look dauntingly large, but once you cut away the trivial stuff (untyped function definitions which have been replaced with types ones, lots and lots of new import statements, etc) the diff is actually not that big.