Skip to content

fix(sorting): also check attributes#1285

Merged
skjnldsv merged 1 commit intomainfrom
fix/sorting
Jul 14, 2025
Merged

fix(sorting): also check attributes#1285
skjnldsv merged 1 commit intomainfrom
fix/sorting

Conversation

@skjnldsv
Copy link
Contributor

Signed-off-by: skjnldsv <skjnldsv@protonmail.com>
@skjnldsv skjnldsv requested a review from susnux July 14, 2025 10:15
@skjnldsv skjnldsv self-assigned this Jul 14, 2025
@skjnldsv skjnldsv added type: bug 🐛 Something isn't working 3. to review 3️⃣ Waiting for reviews labels Jul 14, 2025
@skjnldsv skjnldsv enabled auto-merge July 14, 2025 10:16
...(sortingOptions.sortFoldersFirst ? [(v: INode) => v.type !== 'folder'] : []),
// 3: Use sorting mode if NOT basename (to be able to use display name too)
...(sortingOptions.sortingMode !== FilesSortingMode.Name ? [(v: INode) => v[sortingOptions.sortingMode]] : []),
...(sortingOptions.sortingMode !== FilesSortingMode.Name ? [(v: INode) => v[sortingOptions.sortingMode] || v.attributes[sortingOptions.sortingMode]] : []),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes is clearer, otherwise this is also evaluated if the first value is just falsy like an empty string or the number 0.

Suggested change
...(sortingOptions.sortingMode !== FilesSortingMode.Name ? [(v: INode) => v[sortingOptions.sortingMode] || v.attributes[sortingOptions.sortingMode]] : []),
...(sortingOptions.sortingMode !== FilesSortingMode.Name ? [(v: INode) => v[sortingOptions.sortingMode] ?? v.attributes[sortingOptions.sortingMode]] : []),

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes is clearer, otherwise this is also evaluated if the first value is just falsy like an empty string or the number 0.

Wouldn't that be better then ?
We don't really want to sort my an empty string or a 0 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently it should not make a difference, due to the top level things we have defined. But it could.
E.g. if you have order of value 0 you want to sort by it.

In general if there is a value for that key it should be sorted by it, only if there is no such key on the node it should sort by the key on the node's attributes, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, basically :)
Let me create a followup 👍

@skjnldsv skjnldsv merged commit af33a00 into main Jul 14, 2025
9 checks passed
@skjnldsv skjnldsv deleted the fix/sorting branch July 14, 2025 10:53
@skjnldsv skjnldsv mentioned this pull request Jul 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to review 3️⃣ Waiting for reviews type: bug 🐛 Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants