Skip to content
This repository has been archived by the owner on May 14, 2024. It is now read-only.

Update ldap filter #521

Merged
merged 5 commits into from
Aug 26, 2019
Merged

Update ldap filter #521

merged 5 commits into from
Aug 26, 2019

Conversation

UziTech
Copy link
Member

@UziTech UziTech commented Aug 25, 2019

  • Update ldap-filter
  • Convert escaped characters to Ascii hex code

ldap-filter v0.3.0 has a few breaking changes.

Some of the tests are breaking because they don't allow special characters in an attribute name.

fixes #402

@UziTech
Copy link
Member Author

UziTech commented Aug 25, 2019

It looks like you still need to enable the travis builds on /ldapjs/node-ldapjs/

@UziTech
Copy link
Member Author

UziTech commented Aug 25, 2019

You should also setup appveyor for testing on windows

@jsumners
Copy link
Member

It looks like you still need to enable the travis builds on /ldapjs/node-ldapjs/

I am waiting on a reply for an application to GitHub Actions so that I can setup CI with GitHub CI. Currently just running tests manually where I need to and accepting blindly where it looks good. There's a long road to a new release...

Copy link
Member

@jsumners jsumners left a comment

Choose a reason for hiding this comment

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

Why remove the tests? Shouldn't they instead be updated to correctly test the desired behavior? How about adding a test that explicitly tests the new functionality?

In the future, when updating someone's abandoned PR, please start your branches off of their branches (where feasible). This will include their original work and give them some credit for their attempted efforts.

@jsumners jsumners mentioned this pull request Aug 26, 2019
@UziTech
Copy link
Member Author

UziTech commented Aug 26, 2019

Why remove the tests?

The tests that I removed tested special characters in attribute names which is no longer supported in ldap-filter.

How about adding a test that explicitly tests the new functionality?

There are already tests that cover special characters in the attribute values. I left those alone and made sure those are still passing.

Copy link
Member

@jsumners jsumners left a comment

Choose a reason for hiding this comment

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

LGTM

@jsumners
Copy link
Member

⚠️ This issue has been locked due to age. If you have encountered a recent
problem that seems to be covered by this issue, please open a new issue.

Please include a minimal reproducible example
when opening a new issue.

@ldapjs ldapjs locked as resolved and limited conversation to collaborators Mar 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants