-
Notifications
You must be signed in to change notification settings - Fork 126
Draft: new response handling API #464
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
Modules/LDAPObject.c
Outdated
.name = "_RawLDAPMessage", | ||
.doc = "LDAP Message returned from native code", | ||
.fields = message_fields, | ||
.n_in_sequence = sizeof(message_fields)/sizeof(*message_fields) - 1, |
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.
Set this to 2 to preserve backwards compatibility in result
. (And ideally, at the same time, deprecate using result()
as a tuple, along with resultN
, in favor of the named fields).
This would allow old code to continue using type, data = result()
.
Setting this to a value that will grow as more fields are added defeats the purpose of PyStructSequence
– you might as well use a regular object with getset descriptors.
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 doesn't feel remotely compatible with LDAPObject.result()
anyway, that's one of the reasons the new API is split into a Connection object (which gets a better request API too in a future PR). Not sure whether to make the result* APIs a (heavyweight) wrapper for this or keep it for now.
Keeping n_in_sequence
automatic is an oversight from initial development, thanks for pointing that out, I'll fix it in the next version. What is your take on the Response subclassing logic? I will probably have to replicate it for ExtendedResponse and intermediates, can you see any conceptual issues with how it is written/is it a total hack?
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.
Sadly I won't have time to review the whole PR :(
Could you point to the part I should look at?
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.
Turns out I forgot to add the Python files into the commit so my last comment probably didn't make much sense. I meant the Python bits, this in particular.
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.
At first glance, that looks more complicated than it needs to be. i don't see much advantage over setting the missing attributes to None (or keeping them missing). Is there an advantage?
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.
The subclassing allows one to expose response-specific methods (Compare result, allow parsing the extended result) and if the user is on 3.10, allow pattern matching, on others, make the code more readable, especially if controls were subjected to the same treatment.
abb123e
to
756fba2
Compare
a35462a
to
90652b1
Compare
cedc9c3
to
6b8f56e
Compare
6b8f56e
to
12c3b61
Compare
Uh oh!
There was an error while loading. Please reload this page.