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

Order #1094

Closed
wants to merge 3 commits into from
Closed

Order #1094

wants to merge 3 commits into from

Conversation

auvipy
Copy link
Contributor

@auvipy auvipy commented Oct 16, 2022

Fixes #

Try python dict instead of OrderedDict

Checklist

  • PR only contains one change (considered splitting up PR)
  • unit-test added
  • documentation updated
  • CHANGELOG.md updated (only for user relevant changes)
  • author name in AUTHORS

@auvipy
Copy link
Contributor Author

auvipy commented Oct 16, 2022

probably not a feasible idea yet, or some minor adjustment would fix the failed tests

@sliverc sliverc marked this pull request as ready for review October 16, 2022 19:52
@sliverc sliverc self-requested a review October 16, 2022 19:57
@sliverc
Copy link
Member

sliverc commented Oct 16, 2022

Thanks for the hinter. this is something to consider but I will need to do more research what the drawbacks could be of this change. however your PR does not change it properly as the ordered dict constructur expected a list of tuples. a dict uses the key:value syntax. So you would need to change that as well.

@auvipy
Copy link
Contributor Author

auvipy commented Oct 17, 2022

thanks, I also need some more research / study here.

@sliverc
Copy link
Member

sliverc commented Feb 21, 2023

I had a look into this a bit more and found this article which summarizes the difference between Py 3.7+ dict and OrderedDict quite well.

The main points to use OrderedDict are:

  1. Intent signaling: If you use OrderedDict over dict, then your code makes it clear that the order of items in the dictionary is important. You’re clearly communicating that your code needs or relies on the order of items in the underlying dictionary.
  2. Control over the order of items: If you need to rearrange or reorder the items in a dictionary, then you can use .move_to_end() and also the enhanced variation of .popitem().
  3. Equality test behavior: If your code compares dictionaries for equality, and the order of items is important in that comparison, then OrderedDict is the right choice.

In the JSON:API spec there does not seem to be any specific ordering. So if an API returns attributes as first key and then relationships or the other way around, the two resources are still considered the same. Comparing two objects where the order of the keys are different should actually still be equal, which is not the case when using OrderedDict.

In that case, it could even be considered a bug that we use OrderedDict. Although of course it is great when the order of the keys are always the same for better readability, which is now the case with Python 3.7+ dict anyway.

I try to clean up your PR here, so the tests are green. If you get to it first, feel free to update it yourself.

@sliverc sliverc mentioned this pull request Feb 21, 2023
5 tasks
@sliverc
Copy link
Member

sliverc commented Feb 21, 2023

Seems that you do not have given the permission that I can update this PR. Anyway, it will be good to have another set of eyes look over it. Therefore, I have started continuing this PR in #1132 There are more occurrences of OrderedDict which I am still going through. Closing this PR and let's continue the work in #1132.

@sliverc sliverc closed this Feb 21, 2023
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.

None yet

2 participants