search

rtfeldman / node-test-runner

Runs elm-test suites from Node.js. Get it with npm install -g elm-test
BSD 3-Clause "New" or "Revised" License
132 stars 79 forks source link

Remove reversing step for offline versions #599

Closed jfmengels closed 2 years ago

jfmengels commented 2 years ago

We were sorting then reversing the list in 2 locations. It's more efficient to sort in the opposite order once.

(cc @mpizenberg who did the original implementation)

mpizenberg commented 2 years ago

I don't think the reverse has noticeable impact on the performances. The function was kept in its default order because it's possible in the future to use the increasing order if we want to have an option that picks older versions first. I have such option in elm-test-rs and it's convenient to check that the lower bounds of a package dependencies are correct.

But I don't mind if you want to change the sorting order, I don't have any strong opinion about this.

lydell commented 2 years ago

@mpizenberg Maybe I’m misunderstanding this PR, but as far as I get it, this PR changes nothing it all. It’s purely an optimization:

list
|> sort asc
|> reverse

⬇️ 

list
|> sort desc

Is that right @jfmengels? If so there’s nothing to discuss, right?

mpizenberg commented 2 years ago

Yes, I didn't mean to say it's changing anything. I just meant to say that I initially preferred having one semverCompare function and use it for older versions, or combine with a reversing for newer versions. But in practice, we don't have an option yet to solve with older versions in priority. So it makes sense to use a reverse order sorting instead.