Feature/sort datasets #74
Conversation
src/state/metadata/selectors.ts
Outdated
| // sort function to get the newest datasets on top | ||
| if (a.name === b.name) { | ||
| // if it's the same dataset, return the newest version first | ||
| if (a.version > b.version) { |
There was a problem hiding this comment.
Because version numbers are strings and can be SemVer or CalVer or whateVer, we are probably going to need some better way of doing this comparison. For example, does 2021.10 vs 2021.1 break this?
There was a problem hiding this comment.
yeah it would, i can fix that
There was a problem hiding this comment.
I don't have any great ideas for this except for keeping an internal autoincremented single integer... or having the data creators specify what ordering they want. We rely on the data creators to consistently enforce their own preferred versioning scheme... I guess you could split on "." and hope for the best.
There was a problem hiding this comment.
yeah, I don't think this PR will cover all cases, it's even only ordering the correct dataset group first because it's just reversed alphabetized. Firebase does have time stamps too, so we could keep them ordered by last updated, however, that only works once we have more concrete ways of grouping mega sets together and grouping versions of the same name into one card
src/state/metadata/selectors.ts
Outdated
| if (a.version > b.version) { | ||
| return -1; | ||
| } else { | ||
| return 1; | ||
| } | ||
| // otherwise sort by name | ||
| } else if (a.name > b.name){ | ||
| return -1; | ||
| } else if (a.name < b.name) { | ||
| return 1; | ||
| } else { | ||
| return 0 |
There was a problem hiding this comment.
The sorting function has always been kinda confusing for me, but is it possible to just write return a.version - b.version instead of the if-else statements returning -1, 1, and 0, and just write return a.name - b.name under // otherwise sort by name?
There was a problem hiding this comment.
Maybe it would be return b.version - a.version for newest first? Anyway you see what I'm trying to say.
There was a problem hiding this comment.
yeah, so i've updated it based on Dan's comment, but you're right, I am now comparing numbers so I could just subtract them instead of doing a comparison, that will reduce the number of checks I have to do.
There was a problem hiding this comment.
Sorry, I miss read this comment a bit. For string comparisons you can't subtract them, this comparison is a pseudo alphabetical sort, mostly I just want the same names grouped together.
There was a problem hiding this comment.
I updated the sort function, let me know what you think
src/state/metadata/selectors.ts
Outdated
| if (!patchA || !patchA || patchA === patchB) { | ||
| if (minorANum === minorBNum) { | ||
| // if minor versions are also equal, check patch number | ||
| if (patchA === patchB) { |
There was a problem hiding this comment.
[nit] I don't think lines 42-44 are necessary?
There was a problem hiding this comment.
yeah, I removed them already. You were just too fast on the review :)
| expect(resultDoubleDigit).to.be.lessThan(0); | ||
| }); | ||
| it("returns negative if the patch version of the first item is greater", () => { | ||
| const result = compareVersions("2020.2.2", "2020.1.0"); |
There was a problem hiding this comment.
should this test be 2020.2.2 vs 2020.2.0 ? the minor version is different, preventing the patch version from being tested
Problem
closes: https://aicsjira.corp.alleninstitute.org/browse/CFE-102
Solution
I'm currently sorting them by reverse alphabetical order and by version within dataset name. I thought about using the "isNew" to sort by, but it seems breakable since that's a user defined category. Plus we need to group the datasets by being members of one megaset, and only show the most recent version of each dataset, with the option to load earlier versions, which will both change this code. I added notes to this effect in the selector itself. This solves the temporary problem of getting the datasets in the correct order, but there are a lot of things up in the air and I didn't want to over engineer this just to change it later.
Type of change
Please delete options that are not relevant.
Steps to Verify:
Screenshots (optional):