Skip to content

Allow passing None as uri argument to ldap.initialize() #465

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Aug 6, 2025
Merged

Allow passing None as uri argument to ldap.initialize() #465

merged 1 commit into from
Aug 6, 2025
+3 −2

Conversation

steffen-kiess
Copy link
Contributor

OpenLDAP allows passing NULL to ldap_initialize(). In this case the
default URI from ldap.conf will be used. Allow passing None as uri
argument to ldap.initialize().

@mistotebe
Copy link
Contributor

LGTM, can you also update Doc/reference/ldap.rst?

1 similar comment
@mistotebe
Copy link
Contributor

LGTM, can you also update Doc/reference/ldap.rst?

@steffen-kiess steffen-kiess force-pushed the allow-ldap_initialize_None branch from c091f7f to 0c6d462 Compare April 6, 2022 12:11
@steffen-kiess
Copy link
Contributor Author

LGTM, can you also update Doc/reference/ldap.rst?

I've updated the pull request.

@mistotebe mistotebe force-pushed the allow-ldap_initialize_None branch from 0c6d462 to 9d6ce80 Compare April 6, 2022 13:21
mistotebe
mistotebe previously approved these changes Apr 6, 2022
View reviewed changes
@droideck
Copy link
Contributor

@steffen-kiess could you please rebase the PR with the current master branch?

droideck
droideck previously approved these changes Sep 1, 2022
View reviewed changes
Copy link
Contributor

@droideck droideck left a comment

Choose a reason for hiding this comment

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

Otherwise, looks good to me too. :)

@steffen-kiess steffen-kiess dismissed stale reviews from droideck and mistotebe via 7e6c915 September 2, 2022 09:45
@steffen-kiess steffen-kiess force-pushed the allow-ldap_initialize_None branch from 9d6ce80 to 7e6c915 Compare September 2, 2022 09:45
@steffen-kiess
Copy link
Contributor Author

@steffen-kiess could you please rebase the PR with the current master branch?

I've rebased the commit.

@mistotebe
Copy link
Contributor

@steffen-kiess Reviewing the PR history, some parts of the documentation have gone from the original commit 9d6ce80, is that intentional?

@steffen-kiess steffen-kiess force-pushed the allow-ldap_initialize_None branch from 7e6c915 to c4892e9 Compare September 2, 2022 12:58
@steffen-kiess
Copy link
Contributor Author

@steffen-kiess Reviewing the PR history, some parts of the documentation have gone from the original commit 9d6ce80, is that intentional?

Sorry, no, that was not intentional. I rebased the wrong commit (the one I pushed, not the one updated by you). I've pushed a new rebased commit with the documentation changes.

@steffen-kiess
Copy link
Contributor Author

@mistotebe @droideck Is there anything else I should be doing for this PR?

mistotebe
mistotebe previously approved these changes Dec 15, 2022
View reviewed changes
droideck
droideck previously approved these changes Dec 15, 2022
View reviewed changes
Copy link
Contributor

@droideck droideck left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks!

@tiran
Copy link
Member

tiran commented Nov 6, 2023

I would prefer if you keep the uri argument mandatory and don't default to None. That would require users to explicitly opt-in for default argument with SimpleLDAPObject(None) instead of SimpleLDAPObject().

@steffen-kiess steffen-kiess dismissed stale reviews from droideck and mistotebe via d84deff November 6, 2023 13:37
@steffen-kiess steffen-kiess force-pushed the allow-ldap_initialize_None branch from c4892e9 to d84deff Compare November 6, 2023 13:37
@steffen-kiess
Copy link
Contributor Author

I would prefer if you keep the uri argument mandatory and don't default to None. That would require users to explicitly opt-in for default argument with SimpleLDAPObject(None) instead of SimpleLDAPObject().

Ok, I've changed that so that uri stays mandatory. (And I also rebase to the current main branch).

@droideck
Copy link
Contributor

droideck commented Aug 6, 2025

@steffen-kiess hi! Could you please rebase? I think we should be ready to merge it after that.

@steffen-kiess
Allow passing None as uri argument to ldap.initialize()
a0c2c7d 
OpenLDAP allows passing NULL to ldap_initialize(). In this case the
default URI from ldap.conf will be used. Allow passing None as uri
argument to ldap.initialize().
@steffen-kiess steffen-kiess force-pushed the allow-ldap_initialize_None branch from d84deff to a0c2c7d Compare August 6, 2025 17:18
@steffen-kiess
Copy link
Contributor Author

@steffen-kiess hi! Could you please rebase? I think we should be ready to merge it after that.

Ok, I've rebased to the current main branch.

droideck
droideck approved these changes Aug 6, 2025
View reviewed changes
Copy link
Contributor

@droideck droideck left a comment

Choose a reason for hiding this comment

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

LGTM!
Thank you!

@droideck droideck merged commit 7af3125 into python-ldap:main Aug 6, 2025
16 checks passed
spaceone added a commit to spaceone/python-ldap that referenced this pull request Aug 8, 2025
@spaceone
feat(ldapobject): allow passing uri=None in SimpleLDAPObject
24218ca 
and `ReconnectLDAPObject` to be consistent with `initialize()`.

Fixes: 7af3125
Issue: python-ldap#465
@spaceone spaceone mentioned this pull request Aug 8, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@droideck droideck droideck approved these changes

@mistotebe mistotebe mistotebe left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@steffen-kiess @mistotebe @droideck @tiran