Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Remove unnecessary app version sorting #239

Merged
merged 2 commits into from
Jun 29, 2024
Merged

Conversation

Ushie
Copy link
Member

@Ushie Ushie commented Apr 28, 2024

I couldn't figure out why these lines of code were here, or if removing them would effect anything, but it seems to work perfectly fine
Closes #238

I couldn't figure out why these lines of code were here, or if removing them would effect anything, but it seems to work perfectly fine
Closes #238
@oSumAtrIX oSumAtrIX requested a review from KTibow April 28, 2024 23:40
@KTibow
Copy link
Contributor

KTibow commented May 2, 2024

Real problem is that semver doesn't handle versions with leading 0s, and as such, labels those as invalid.

I couldn't figure out why these lines of code were here

To sort all the invalid versions to the start I think

@oSumAtrIX
Copy link
Member

What do you mean? The version is not according to semver specs.

@KTibow
Copy link
Contributor

KTibow commented May 2, 2024

Yes, yet we rely on the semver package for sorting the items.

@oSumAtrIX
Copy link
Member

That should be removed then

@KTibow
Copy link
Contributor

KTibow commented May 2, 2024

removed

Are you saying sorting the supported versions should be entirely disabled? Or to replace it instead of entirely removing it?

@oSumAtrIX
Copy link
Member

oSumAtrIX commented May 2, 2024

I am saying the version should not be parsed as semver

@KTibow
Copy link
Contributor

KTibow commented May 2, 2024

Well, please explicitly state if you mean it should be parsed another way (most likely) or not parsed (literal interpretation of what you're saying)

@oSumAtrIX
Copy link
Member

There's no requirement for sorting via the version name

@Ushie
Copy link
Member Author

Ushie commented May 3, 2024

How else do you propose we sort it?

@oSumAtrIX
Copy link
Member

As stated in my previous comment, there is no requirement to sort by version name at all.

@KTibow
Copy link
Contributor

KTibow commented May 3, 2024

Are you saying we should use the sorting provided by the backend instead? If so, that sounds like you should make a separate issue for that

@oSumAtrIX
Copy link
Member

The backend does not specify any sorting and is considered to be random. No issue is necessary

@KTibow
Copy link
Contributor

KTibow commented May 3, 2024

What I'm saying is if you want to remove the current sorting, you should discuss that in a place like an issue first.

@oSumAtrIX
Copy link
Member

oSumAtrIX commented May 3, 2024

Since it's discussed here with a clear solution, that won't be necessary anymore and this PR can be repurposed to solve both issues at once

@Ushie Ushie changed the title fix: Sort versions consistently fix: Remove unnecessary sorting Jun 19, 2024
@Ushie Ushie changed the title fix: Remove unnecessary sorting fix: Remove unnecessary app version sorting Jun 19, 2024
@Ushie Ushie removed the request for review from KTibow June 19, 2024 15:24
@oSumAtrIX oSumAtrIX merged commit 345158e into dev Jun 29, 2024
1 of 2 checks passed
@oSumAtrIX oSumAtrIX deleted the fix/app-version-ordering branch June 29, 2024 19:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: Patches List - Sorting of YouTube's Compatible Versions (Expanded List)
3 participants