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

Maintain order of original data #9

Merged
merged 2 commits into from
Jan 11, 2018
Merged

Maintain order of original data #9

merged 2 commits into from
Jan 11, 2018

Conversation

jpmckinney
Copy link
Member

closes #8

I'm not sure what the function of the sorted is. If it serves a function, there should be a test that fails without it.

@kindly
Copy link
Contributor

kindly commented Jan 10, 2018

@jpmckinney
This is very tricky to test as python dictionaries are inherently not ordered so it could pass sometimes and fail others for the same test. I have had tests that fail about 1 in 10 times or some people and not because of random dictionary ordering.
The aim of the sort was to make sure longer paths come after shorter ones otherwise the unflattering will not work.

However, to make sure this is correct we should add OrderedDicts instead of plain dictionaries in these places:

https://github.com/open-contracting/ocds-merge/blob/master/ocdsmerge/merge.py#L170
https://github.com/open-contracting/ocds-merge/blob/master/ocdsmerge/merge.py#L199

then we definitely do not have to worry about the sorting and it will keep order.

@jpmckinney
Copy link
Member Author

I've updated those two lines now.

@jpmckinney jpmckinney merged commit 77a4ee8 into master Jan 11, 2018
@jpmckinney jpmckinney deleted the order branch January 11, 2018 16:59
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.

Don't reorder fields from original data
2 participants