[python-ldap] Add type hints #522

Alphix · 2023-05-15T19:57:42Z

With this patch applied, MYPYPATH=/Stubs mypy --strict Lib/ passes without any warnings. tox also passes for everything from py37 to py11 (and also including doc and pypy3).

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.

Alphix · 2023-05-16T07:30:53Z

With regard to the CI failures, I should add that py36 fails even without this patch applied. And pypy passes on my machine, the failure seems related to how the CI is configured...

With this patch applied:

~/checkouts/python-ldap$ tox
...
  py36: FAIL code -11 (0.50 seconds)
  py37: OK (14.72=setup[4.62]+cmd[10.10] seconds)
  py38: OK (13.79=setup[3.64]+cmd[10.14] seconds)
  py39: OK (14.70=setup[4.61]+cmd[10.09] seconds)
  py310: OK (14.46=setup[4.70]+cmd[9.76] seconds)
  py311: OK (12.52=setup[3.19]+cmd[9.33] seconds)
  c90-py36: FAIL code 1 (3.13 seconds)
  c90-py37: OK (15.26=setup[5.13]+cmd[10.12] seconds)
  py3-nosasltls: OK (10.38=setup[0.01]+cmd[0.11,1.48,0.29,8.50] seconds)
  doc: OK (9.03=setup[4.59]+cmd[0.13,0.04,3.01,1.26] seconds)
  py3-trace: OK (13.60=setup[4.33]+cmd[9.27] seconds)
  pypy3: OK (17.25=setup[6.45]+cmd[10.80] seconds)
  evaluation failed :( (139.37 seconds)

~/checkouts/python-ldap$ MYPYPATH=Stubs/ mypy --strict Lib
mypy.ini: No [mypy] section in config file
Success: no issues found in 40 source files

mistotebe · 2023-07-21T10:42:11Z

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, ldap.pkginfo changes, compat.py removal, etc. in any order that makes sense and then just adding the type hints only?

And just out of curiosity, is there a reason you chose to use <type> | None rather than Optional[<type>]?

tiran

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 ldapurl and ldif into the ldap package eventually. I suggest ldap.types module.
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.typed file and add the file to MANIFEST.in
Move mypy.ini content to either pyproject.toml or setup.cfg
Extend tox.ini with a mypy check for every supported Python version
Add _ldap.pyi to MANIFEST.in
Would it make sense to move _ldap.pyi to Lib/_ldap.pyi and 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

intgr · 2023-10-01T20:04:23Z

Hi! Why has this this work stalled? @Alphix are you still interested in continuing this?

Fix Python 3.6 compatibility issues. We still support old Python versions for $REASONS..., sorry.

What are those reasons? Many projects have already dropped 3.7.

Alphix · 2023-10-01T21:21:25Z

Hi! Why has this this work stalled? @Alphix are you still interested in continuing this?

@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...

Fix Python 3.6 compatibility issues. We still support old Python versions for $REASONS..., sorry.

What are those reasons? Many projects have already dropped 3.7.

My problem is that tox py36 fails even without the patch applied, haven't looked into that yet...

tiran · 2023-10-02T08:41:32Z

Fix Python 3.6 compatibility issues. We still support old Python versions for $REASONS..., sorry.

What are those reasons? Many projects have already dropped 3.7.

Because we (downstream vendor) still support Python 3.6.

intgr · 2023-10-02T15:08:08Z

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)?

Alphix · 2023-10-05T21:04:10Z

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 tox py36 work for anyone else? Without this patch applied? Because it doesn't for me....

tiran · 2023-10-06T05:37:48Z

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 tox py36 work for anyone else? Without this patch applied? Because it doesn't for me....

tox -e py36 is passing on my Fedora machine. It's also working fine on our CI, which runs the tests both on Ubuntu and Fedora.

Alphix · 2024-01-24T15:08:17Z

Looks good, would you be able to split it into a few more commits each doing self-contained changes: any fixes, ldap.pkginfo changes, compat.py removal, etc. in any order that makes sense and then just adding the type hints only?

Sorry, but I think that would just be way too much work at the moment. The changes to the code mostly go hand-in-hand with the typing annotations (and in most cases are just there to make the types more explicit to mypy).

And just out of curiosity, is there a reason you chose to use <type> | None rather than Optional[<type>]?

That was an oversight, didn't realise it was relevant for backwards compatibility, I've fixed that in the second version.

Alphix · 2024-01-24T15:12:16Z

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 ldapurl and ldif into the ldap package eventually. I suggest ldap.types module.

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.typed file and add the file to MANIFEST.in

Move mypy.ini content to either pyproject.toml or setup.cfg

Extend tox.ini with a mypy check for every supported Python version

Add _ldap.pyi to MANIFEST.in

Would it make sense to move _ldap.pyi to Lib/_ldap.pyi and install it along the shared extension?

@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 mypy is not enabled for the py312 environment right now because it needs different compile flags. Not sure what the best way to handle that in tox.ini would be....but perhaps it could be addressed in a separate patch?

Also, I should add that this is just a starting point for type annotations. There are still a bunch of uses of Any and type: ignore in the code. I already have another branch which gradually fixes up those and makes the type checking stricter (but that's for later).

This is in preparation for later patches which add typing.

Alphix · 2024-01-26T12:52:12Z

Looks good, would you be able to split it into a few more commits each doing self-contained changes: any fixes, ldap.pkginfo changes, compat.py removal, etc. in any order that makes sense and then just adding the type hints only?

Sorry, but I think that would just be way too much work at the moment. The changes to the code mostly go hand-in-hand with the typing annotations (and in most cases are just there to make the types more explicit to mypy).

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 git add -p in the process).

~~So you might want to hold off on the review for now...~~

This is also in preparation for the typing patches as it helps to avoid import loops.

Redefining variables with varying types makes mypy unhappy.

The WrongResultType will receive an Interable (set) of integers representing the expected result types, doing a str.join() on that set doesn't work. Also, make sure that startSearch() has been called before processResults() is called which, in turn, ensures that self._msgId is not None.

First, __contains__() can be called with anything as the key, but only supports str keys (due to the key.lower() call), so make sure that the key is actually a str. Second, strlist_union() is described as returning a list, but actually returns an iterable, so make it return a list. @@ -134,4 +136,4 @@ def strlist_union(a,b):

Make sure that c_template is defined in the base class (it is already defined in all subclasses) so that it is always present. Also, fix a spelling mistake.

First, change class PersistentSearchControl(RequestControl) so that self.changeTypes is always a List[str] instead of List[str|int]. Second, in class EntryChangeNotificationControl, change decodeControlValue() to not return a value (see the ResponseControl parent class).

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.

Alphix · 2024-01-27T17:08:23Z

Looks good, would you be able to split it into a few more commits each doing self-contained changes: any fixes, ldap.pkginfo changes, compat.py removal, etc. in any order that makes sense and then just adding the type hints only?

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 mypy into the build machinery.

intgr

Awesome, thanks for your work. I tried it out in my project and have some feedback.

intgr · 2024-01-29T10:00:30Z

Lib/ldap/controls/libldap.py

-KNOWN_RESPONSE_CONTROLS[ldap.CONTROL_ASSERT] = AssertionControl
+# FIXME: This is a request control though?
+#KNOWN_RESPONSE_CONTROLS[ldap.CONTROL_ASSERT] = AssertionControl


FIXME should be fixed. Just pointing it out, not sure what to do here.

Yeah, I'm not sure that I'm right here, so I kind of left it as a marker for the maintainers to have a look. I guess the "fix" would be just deleting the lines, but not sure

Lib/ldap/types.py

Lib/_ldap.pyi

setup.py

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

…p/controls/* Several of the request controls are registered in KNOWN_RESPONSE_CONTROLS, which appears to be an oversight...probably 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.

Alphix · 2024-01-30T20:55:33Z

Ok, I've pushed a new version taking @intgr's comments into account. Most controversial change here is the move of _ldap to ldap._ldap (which is why that's a separate patch)

Alphix · 2024-04-03T18:25:09Z

@tiran gentle ping?

macserv · 2024-11-27T19:36:51Z

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.

Alphix · 2024-11-28T08:01:08Z

Hey, @Alphix, can you update this to resolve conflicts, and then page @droideck, @mistotebe, and @tiran for review?

@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 mentioned this pull request May 16, 2023

Add PEP 484 -- Type Hints #368

Open

tiran requested changes Jul 21, 2023

View reviewed changes

Alphix force-pushed the type-hints-hack branch 2 times, most recently from 41ffdf3 to 8b4736f Compare January 24, 2024 15:02

[python-ldap] Remove compatibility wrappers for Py2/Py3

6c3530a

This is in preparation for later patches which add typing.

Alphix added 14 commits January 27, 2024 18:00

[python-ldap] Make __version__ imports more consistent

d80e4be

This is also in preparation for the typing patches as it helps to avoid import loops.

[python-ldap] Type fixes for Lib/ldap/__init__.py

fad0b14

Redefining variables with varying types makes mypy unhappy.

[python-ldap] Type fixes for Lib/ldap/constants.py

99a71b8

Make sure that c_template is defined in the base class (it is already defined in all subclasses) so that it is always present. Also, fix a spelling mistake.

[python-ldap] Type fixes for Lib/ldap/controls/psearch.py

c5512e3

decodeControlValue() expects a binary encodedControlValue, so correct the comparison to reflect that.

[python-ldap] Type fixes for Lib/ldap/controls/sss.py

48d0949

Avoid redefining a variable to a different type.

[python-ldap] Type fixes for Lib/ldap/extop/__init__.py

6cb5e19

This helps to avoid import loops in later patches.

[python-ldap] Type fixes for Lib/ldap/modlist.py

4775c56

This helps with the type checking later.

[python-ldap] Type fixes for Lib/ldap/resiter.py

a754932

Make class ResultProcessor an explicit subclass of LDAPObject This keeps type checkers happy but shouldn't have any functional difference.

[python-ldap] Type fixes for Lib/ldap/sasl.py

7ab5fc5

Avoid redefining variables to keep type checkers happy.

Alphix added 2 commits January 27, 2024 18:00

[python-ldap] Type fixes for Lib/ldapurl.py

66e130c

__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.

[python-ldap] Type fixes for Lib/slaptest/*

a37570d

Mostly some explicit None checks, avoiding variable redefinition and the removal of a circuitous import.

Alphix force-pushed the type-hints-hack branch from 8b4736f to e203e04 Compare January 27, 2024 17:04

intgr reviewed Jan 29, 2024

View reviewed changes

Alphix requested a review from tiran January 29, 2024 19:40

Alphix added 13 commits January 30, 2024 20:42

[python-ldap] Type fixes for Lib/ldap/ldapobject.py

2f91645

Mostly some small changes to avoid variable redefinition and changing some functions to remove superfluous return values.

[python-ldap] Type fixes for Lib/ldap/controls/openldap.py

9a0856a

Small fix to keep type checkers happy.

[python-ldap] Type fixes for Lib/ldap/controls/pagedresults.py

6c08182

Essentially, make sure that the cookie is always stored as binary

[python-ldap] Type fixes for Lib/ldap/extop/__init__.py

c1a44b4

self.responseValue is bytes, not a str

[python-ldap] Improve documentation in Lib/ldap/dn.py

14a305d

[python-ldap] Correct KNOWN_RESPONSE_CONTROLS registration in Lib/lda…

ec5dc51

…p/controls/* Several of the request controls are registered in KNOWN_RESPONSE_CONTROLS, which appears to be an oversight...probably due to a bit too much copy-pasting?

[python-ldap] Add type annotations

1897ba8

Phew, after all the prep work, the actual type annotations can finally be added.

[python-ldap] Integrate typing into the build/CI system

1b51ea1

[python-ldap] Move _ldap to ldap._ldap

c30644e

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.

[python-ldap] Add stubtest to typing CI

ba337e8

This helps in ensuring that the stub and the C module don't drift apart.

Alphix force-pushed the type-hints-hack branch from e203e04 to ba337e8 Compare January 30, 2024 20:46

[python-ldap] Add type hints #522

Are you sure you want to change the base?

[python-ldap] Add type hints #522

Uh oh!

Conversation

Alphix commented May 15, 2023

Uh oh!

Alphix commented May 16, 2023

Uh oh!

mistotebe commented Jul 21, 2023

Uh oh!

tiran left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

intgr commented Oct 1, 2023

Uh oh!

Alphix commented Oct 1, 2023

Uh oh!

tiran commented Oct 2, 2023

Uh oh!

intgr commented Oct 2, 2023

Uh oh!

Alphix commented Oct 5, 2023

Uh oh!

tiran commented Oct 6, 2023

Uh oh!

Alphix commented Jan 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Alphix commented Jan 24, 2024

Uh oh!

Alphix commented Jan 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Alphix commented Jan 27, 2024

Uh oh!

intgr left a comment

Choose a reason for hiding this comment

Uh oh!

intgr Jan 29, 2024

Choose a reason for hiding this comment

Uh oh!

Alphix Jan 30, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Alphix commented Jan 30, 2024

Uh oh!

Alphix commented Apr 3, 2024

Uh oh!

macserv commented Nov 27, 2024

Uh oh!

Alphix commented Nov 28, 2024

Uh oh!

Uh oh!

tiran left a comment •

edited

Loading

Alphix commented Jan 24, 2024 •

edited

Loading

Alphix commented Jan 26, 2024 •

edited

Loading