search

inejge / ldap3

A pure-Rust LDAP library using the Tokio stack
Apache License 2.0
220 stars 38 forks source link

Update nom to the current version 7 #93

Closed dequbed closed 1 year ago

dequbed commented 1 year ago

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.

inejge commented 1 year ago

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.)

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 commented 1 year ago

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 commented 1 year ago

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 :100: