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

Use a stable sort in Patch.applyAll #126

Merged
merged 1 commit into from Nov 5, 2019

Conversation

@jathak
Copy link
Member

jathak commented Nov 5, 2019

Patches can be order-dependent when insertion patches are involved, so
we need to use a stable sort in Patch.applyAll.

@jathak jathak requested a review from nex3 Nov 5, 2019
@nex3
nex3 approved these changes Nov 5, 2019
var sortedPatches = patches.toList()
..sort((a, b) => a.selection.compareTo(b.selection));
var sortedPatches = patches.toList();
mergeSort(sortedPatches,

This comment has been minimized.

Copy link
@nex3

nex3 Nov 5, 2019

Contributor

Add a comment here explaining why you're using mergeSort() over List.sort().

..sort((a, b) => a.selection.compareTo(b.selection));
var sortedPatches = patches.toList();
mergeSort(sortedPatches,
compare: (a, b) => a.selection.compareTo(b.selection));

This comment has been minimized.

Copy link
@nex3

nex3 Nov 5, 2019

Contributor

I don't think you need to pass this, I think calling compareTo() is the default behavior.

Patches can be order-dependent when insertion patches are involved, so
we need to use a stable sort in Patch.applyAll.
@jathak jathak force-pushed the stable-sort branch from bdecfd2 to d03be50 Nov 5, 2019
@jathak jathak merged commit 0e1d6ab into master Nov 5, 2019
1 check passed
1 check passed
Travis CI - Pull Request Build Passed
Details
@jathak jathak deleted the stable-sort branch Nov 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.