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
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

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

wants to merge 1 commit into from

Conversation

steffen-kiess
Copy link

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
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
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
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
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
Allow passing None as uri argument to ldap.initialize()
d84deff 
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 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
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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mistotebe mistotebe mistotebe left review comments

@droideck droideck droideck left review comments

At least 1 approving review is required to merge this pull request.

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