Fix sort ordering logic #696
Conversation
|
thanks for that! we will push it. |
src/app/execution/txHelper.js
Outdated
| // If we reach here, either a and b are both constant or both not; sort by name then | ||
| // special case for fallback and constructor | ||
| if (a.type === 'function') { | ||
| return a.name.localeCompare(b.name) |
There was a problem hiding this comment.
Hm, this shouldn't be locale specific I think. Also identifiers can be ASCII only.
There was a problem hiding this comment.
localeCompare() is the only native string-comparison function in Javascript; would you rather multiple separate calls to check greater, less, and equal?
There was a problem hiding this comment.
Since this is only used for display purposes it should be fine, though I'd add a comment.
If this would be critical piece of code, either:
- a manual implementation
- or setting the locale to ASCII English upfront
would be needed, because certain languages consider di/trigraphs with different sorting order.
|
@MidnightLightning are you interested in finishing this? Would be a good feature to merge. |
MidnightLightning
force-pushed
the
patch-1
branch
from
a812f36
to
a7df88a
Compare
|
@axic Thanks for the reminder; updated now! |
|
@MidnightLightning could you rebase? |
MidnightLightning
force-pushed
the
patch-1
branch
from
a7df88a
to
ec8d84b
Compare
|
@yann300 Rebased now. |
|
sorry, there are some |
|
@yann300 Coding standard errors fixed now. |
|
great! thanks |
|
@yann300 Any further changes needed on this one? GitHub's still indicating you as a reviewer have changes requested. |
| @@ -32,20 +32,20 @@ module.exports = { | |||
| }, | |||
|
|
|||
| sortAbiFunction: function (contractabi) { | |||
There was a problem hiding this comment.
Can you please add a comment above here explaining the order? e.g. constructor, fallback, constant functions, non-constant functions?
There was a problem hiding this comment.
@axic Okay, added now.
|
@yann300 anything left on this? |
|
moved, thanks @MidnightLightning |
Having two separate
sortcalls doesn't actually preserve the ordering of the first sort (by name) so only the second (constant vs. not) was being sorted on properly. This fix first checks to see if the two items have differentconstantvalues, and only if theirconstantvalues are the same does it then sort by name.