Sort run names with leading numbers differently #6664
Conversation
| AFTER, | ||
| } | ||
|
|
||
| interface SortOptions { |
There was a problem hiding this comment.
Why do we need this interface with a single attribute. Is there a reason you cannot just use the enum directly everywhere this interface is used?
There was a problem hiding this comment.
This is a pretty complicated function and I thought that using an object as a parameter would make the options provided more readable.
There was a problem hiding this comment.
I am ok with that justification. I think I would prefer naming the enum something more readable....UndefinedValuePlacement?
I cant really think of something good. Maybe this is best.
| }); | ||
| return sortedItems; | ||
|
|
||
| function orderFromLocalComparison( |
There was a problem hiding this comment.
I do not understand the name of this function. Maybe just call it compareValues?
There was a problem hiding this comment.
Alright, compareValues it is.
| ]); | ||
| }); | ||
|
|
||
| it('places undefined values at the end', () => { |
There was a problem hiding this comment.
If you are going to have the ability to place undefined values before(which I am not sure we really need). Then you should have a test for it.
There was a problem hiding this comment.
We kinda have that already, that is what sorts runs with purely numeric run names before runs with leading numbers is checking
There was a problem hiding this comment.
I can see how that test along with returns undefined when a non numeric value is provided kind of tests for it.
| @@ -28,6 +28,7 @@ describe('sorting utils', () => { | |||
| expect(parseNumericPrefix('0')).toEqual(0); | |||
| expect(parseNumericPrefix('123')).toEqual(123); | |||
| expect(parseNumericPrefix('123/')).toEqual(123); | |||
| expect(parseNumericPrefix('123/foo')).toEqual(123); | |||
There was a problem hiding this comment.
This still has a '/' after the number. I was thinking something like 123train. Still just a nit though.
There was a problem hiding this comment.
Ahh, okay. I'll add that case too
## Motivation for features / changes We have been treating all run names as strings even though some run names are (serialized) numbers and some begin with numbers. This means that sorting worked pretty unintuitively. Note: I've also changed the behavior around sorting `undefined` values. When sorting descending they should now appear at the top of the list. This was a recent change but it wasn't clear it was intentional and I found it made the code more complex. tensorflow#6651 Internal users see [b/278671226](http://b/278671226) ## Screenshots of UI changes (or N/A) Before:  After:  
## Motivation for features / changes We have been treating all run names as strings even though some run names are (serialized) numbers and some begin with numbers. This means that sorting worked pretty unintuitively. Note: I've also changed the behavior around sorting `undefined` values. When sorting descending they should now appear at the top of the list. This was a recent change but it wasn't clear it was intentional and I found it made the code more complex. #6651 Internal users see [b/278671226](http://b/278671226) ## Screenshots of UI changes (or N/A) Before:  After:  
Motivation for features / changes
We have been treating all run names as strings even though some run names are (serialized) numbers and some begin with numbers. This means that sorting worked pretty unintuitively.
Note: I've also changed the behavior around sorting
undefinedvalues. When sorting descending they should now appear at the top of the list. This was a recent change but it wasn't clear it was intentional and I found it made the code more complex.#6651
Internal users see b/278671226
Screenshots of UI changes (or N/A)
Before:
After:
