Skip to content
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

Fix for #31 #35

Closed
wants to merge 1 commit into from
Closed

Fix for #31 #35

wants to merge 1 commit into from

Conversation

bhuism
Copy link

@bhuism bhuism commented Dec 19, 2015

No description provided.

@bhuism
Copy link
Author

bhuism commented Jan 19, 2016

@rwinch, would it be possible to merge this PR? I have signed de 'Individual Contributor Agreement'

@rwinch
Copy link
Member

rwinch commented Mar 28, 2016

@bhuism Thanks for the PR!

Unfortuantely, we cannot merge non passive changes. Since this modifies a public interface we cannot merge the pull request. If you can come up with a passive way of resolving this issue we can look into getting this merged.

@rwinch rwinch closed this Mar 28, 2016
@rwinch rwinch self-assigned this Mar 28, 2016
@bhuism
Copy link
Author

bhuism commented Mar 28, 2016

@rwinch thanks! the interface is not modified, the behavior is modified.

Is it allowed to change the the behavior of a method?

If not how should #31 be fixed in your opinion?

@rwinch
Copy link
Member

rwinch commented Mar 28, 2016

@bhuism Thanks for the fast response.

Perhaps I am missing something due to only looking in the diff, but it appears that a new method was added to LdapOperations. This constitutes as a change because anyone implementing the interface must now add a new method.

@bhuism
Copy link
Author

bhuism commented Mar 28, 2016

Ah, I got ya, I'll try to make a PR another way, thanks!

@bhuism
Copy link
Author

bhuism commented Mar 29, 2016

@rwinch would it be ok to add the new method to LdapOperations in 2.1.0? in stead of 2.0.x? I can't fix it cleanly without the interface change.

@rwinch
Copy link
Member

rwinch commented Mar 29, 2016

@bhuism We cannot add a method until 3.x since we follow APR Versioning

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants