_unparseEntryRecord now handles OrderedDict #245
Conversation
Codecov Report
@@ Coverage Diff @@
## master #245 +/- ##
======================================
Coverage 71.1% 71.1%
======================================
Files 49 49
Lines 4818 4818
Branches 812 812
======================================
Hits 3426 3426
- Misses 1056 1057 +1
+ Partials 336 335 -1
Continue to review full report at Codecov.
|
|
Can you add a test to show the intended behavior and make sure it doesn't break in the future? |
|
It simply doesn't sort the values before processing This makes an ordereddict, or a simple dict, to be processed as is without reordering of its member. |
|
I think I understand what the issue is. But if you want to rely on this
feature, it needs some tests to ensure that it works on all supported
configurations and that we don't accidentally break it in the future.
That's currently blocking this pull request.
…On Wed, Oct 10, 2018, 16:11 Giuseppe De Marco ***@***.***> wrote:
It simply doesn't sort the values before processing
8a752f9#diff-384b211fbc9bab0d31d7e3fb02142ca2L153
<8a752f9#diff-384b211fbc9bab0d31d7e3fb02142ca2L153>
This makes an ordereddict, or a simple dict, to be processed as is without
reordering of its member.
Did I have well understand the question?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#245 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AASfSn-BkFl59hbpCk4T3L7r_wCLd97Lks5ujgAtgaJpZM4XDowa>
.
|
|
Another thing to consider is the effect on interpreters where dict is not
ordered. AFAICS, this will make the output unpredictable there, and I don't
think that's what we want.
Maybe limit this to OrderedDict only?
…On Wed, Oct 10, 2018, 16:11 Giuseppe De Marco ***@***.***> wrote:
It simply doesn't sort the values before processing
8a752f9#diff-384b211fbc9bab0d31d7e3fb02142ca2L153
<8a752f9#diff-384b211fbc9bab0d31d7e3fb02142ca2L153>
This makes an ordereddict, or a simple dict, to be processed as is without
reordering of its member.
Did I have well understand the question?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#245 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AASfSn-BkFl59hbpCk4T3L7r_wCLd97Lks5ujgAtgaJpZM4XDowa>
.
|
|
Probably it should better to know why a dict should be sorted on its keys
before processing?
Sure, I really don't know why this was coded this way and the impact of
this on the current implementation.
I just know that: using an ordereddict I'm able to export an ldiff using a
quite common ldap ordering, because I choose the order in an arbitrary way.
This patch works with dict and ordereddict and I'm using it in production
environment. Use it, if you need and understand the usage I proposed, as it come
|
On some of the Python versions python-ldap supports, dict order is unspecified or even random. I'd be fine with skipping the sorting for OrderedDict only – using OrderedDict is a nice explicit way to say you care about the order, and it's something you don't get directly from LDAP. (FWIW, cases like this are why OrderedDict is not deprecated in Python 3.6+.)
Great! I'm glad python-ldap is helping! |
There was a problem hiding this comment.
The change is going to break existing software that relies on the fact that LDIF Writer sorts output to minimize diffs. As @encukou mentioned in his comment ordinary dicts should be sorted. I'd be fine with special casing of collections.OrderedDict, though. This also needs a test.
No description provided.