-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
- Loading branch information
1 parent
8679ea9
commit 145ba88
Showing
1 changed file
with
5 additions
and
2 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
145ba88
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not really sort anything. But then I re-tested https://github.com/bmwiedemann/theunreproduciblepackage/blob/master/hash/hash.py and found that python3 already seems to sort it internally.
145ba88
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(This comment isn't on the PR.)
Sort what? Python has order-preserving dicts (as an implementation detail starting Python 3.6, and as an official language specification requirement starting 3.7), which aren't the same as sorted dicts, but do mean that the output is as deterministic as the input (in this case, json from a subprocess).
145ba88
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variable names
unsorted_deps
->deps
implies there is some sorting going on, but all that happens is an unnecessary copy.145ba88
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that really should be
deps = dict(sorted(unsorted_deps.items(), key=lambda x: x[0]))
edit: I left this are review on the actual PR