Page MenuHomePhabricator

Sort inline comments by id in case of ties

Authored by sophiebits on Apr 4 2014, 1:08 AM.


Group Reviewers
Blessed Reviewers
Restricted Diffusion Commit
rPf9a92c7631aa: Sort inline comments by id in case of ties

This ensures that two comments by the same author on the same line are sorted properly.

Test Plan

Before this patch, made two comments that appeared in the wrong order. With this patch, they sort correctly.

Diff Detail

rP Phabricator
Lint Skipped
Unit Tests Skipped

Event Timeline

sophiebits updated this revision to Diff 20626.Apr 4 2014, 1:08 AM
sophiebits retitled this revision from to Sort inline comments by id in case of ties.
sophiebits updated this object.
sophiebits edited the test plan for this revision. (Show Details)
sophiebits added a reviewer: epriestley.
Herald added subscribers: Korvin, epriestley.
sophiebits added inline comments.Apr 4 2014, 1:08 AM

Are these casts unnecessary now?

epriestley edited edge metadata.Apr 4 2014, 1:36 AM

I think the inlines passed in here always share the same author, so sorting by length doesn't actually make a ton of sense. What do you think about just <num, id>?

(The casts are unnecessary with sprintf().)

(Even if, in the future, inlines by different authors are passed in here, sorting by id seems better in all cases than len to me, I think?)

Well, you can have overlapping comment ranges, right? So I could have four comments like:

num  len  id
10   0    1001
10   0    1003
10   5    1002
10   5    1004

and they should be displayed in that order.

sophiebits updated this revision to Diff 20628.Apr 4 2014, 1:40 AM
sophiebits edited edge metadata.

Remove casts

epriestley accepted this revision.Apr 4 2014, 1:41 AM
epriestley edited edge metadata.


This revision is now accepted and ready to land.Apr 4 2014, 1:41 AM
epriestley closed this revision.Apr 4 2014, 1:42 AM
epriestley updated this revision to Diff 20629.

Closed by commit rPf9a92c7631aa (authored by @spicyj, committed by @epriestley).