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

Update nom to the current version 7 #93

Merged
merged 1 commit into from
Nov 20, 2022

Conversation

dequbed
Copy link

@dequbed dequbed commented Nov 16, 2022

How the turn tables :)

Anyway, given that I left you with that mess of code to begin with I took the liberty to upgrade asnom's/lber's dependencies a bit. ;)

The crate still needs to be replaced with something proper, and this PR very much isn't that rewrite, but this update was quick to do.

My terrible tests in lber still pass and I'm rather sure that the updated code does close enough the same to the original that everything will just continue to work, but it's a breaking API change, so you potentially want to roll this into the next breaking change in ldap3 too.

Comment on lines +24 to +25
#[cfg(feature = "x509-parser")]
use x509_parser::nom::Parser;
Copy link
Owner

Choose a reason for hiding this comment

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

This import is unused and breaks CI.

@inejge
Copy link
Owner

inejge commented Nov 16, 2022

Hey, this is a very pleasant surprise indeed 🙂 Thank you for the PR! You're right about bumping up both libraries to the next breaking-change version, of course. And while you may self-critically call lber a "mess", I call it "working code", and I firmly believe in working code. (I may replace it, eventually, but it's been working all these years, so there was no urgency.)

The PR fails in CI for two reasons: an unused import (referenced above) and formatting (I routinely use rustfmt on the code.) I'd like to have at least the CI failure fixed before merging. Also, the commit message would autoclose #92, but the PR addresses only the lber part of the nom upgrade, so I'd prefer to have that reference removed. (I'm going to deal with the filter part myself.) If you make further changes, please squash them in a single commit and force-push it.

@dequbed
Copy link
Author

dequbed commented Nov 20, 2022

Hey, this is a very pleasant surprise indeed slightly_smiling_face Thank you for the PR! You're right about bumping up both libraries to the next breaking-change version, of course. And while you may self-critically call lber a "mess", I call it "working code", and I firmly believe in working code. (I may replace it, eventually, but it's been working all these years, so there was no urgency.)

Honestly, good call. When not busy by other work I spend some good time over the last few years trying to build a good and efficient parser for the LDAP BER dialect and while I learned a lot about (zero-copy) parsing and related, it didn't exactly result in much feature work happening.

The PR fails in CI for two reasons: an unused import (referenced above) and formatting (I routinely use rustfmt on the code.) I'd like to have at least the CI failure fixed before merging. Also, the commit message would autoclose #92, but the PR addresses only the lber part of the nom upgrade, so I'd prefer to have that reference removed. (I'm going to deal with the filter part myself.) If you make further changes, please squash them in a single commit and force-push it.

These sound like very reasonable changes. I probably won't have time today but I'll make sure to fix this PR in the coming week :)

@inejge inejge merged commit ec6eca6 into inejge:master Nov 20, 2022
@inejge
Copy link
Owner

inejge commented Nov 20, 2022

Ok, my filter port was easier than expected, so I went ahead and merged the PR as is, then fixed it up. Aside from the import and formatting, several tests were failing, all ultimately dependent on minimal integer encoding which was missing from structures/integer.rs (see c261daf.)

Thanks again for the PR, much appreciated 💯

@dequbed dequbed deleted the feature/update-deps branch January 6, 2023 12:12
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