jsdelivr / libgrabber

DEPRECATED Keeps projects hosted in jsDelivr updated
20 stars 21 forks source link

Fix jsdelivr/jsdelivr#9553 #61

Closed MartinKolarik closed 8 years ago

lewisgoddard commented 8 years ago

You're going to fix this by replacing a library, rather than putting a pull request in there?

megawac commented 8 years ago

@MartinKolarik why don't you just send this a patch to https://github.com/megawac/semvish? It was literally written for libgrabber. I would be hesitant to merge this in as it may have other consequences (i.e. regressions in version support)

megawac commented 8 years ago

I would also consider this a bug in underscore.strings natural compare.

Anyway @MartinKolarik its a very simple fix which you can implement by changing this loop to convert strings to numbers...

MartinKolarik commented 8 years ago

@megawac I agree this particular case would be easy to fix in semvish, but it is not the only problem I encountered using it. I switched to this sorting algorithm in api after discovering about 40 projects had empty versions because semvish couldn't parse them.

I would normally send a PR to semvish, but I don't believe semvish is the best solution in this case. It relies on semver, and some versioning schemes simply can't be normalized to semver (e.g. what do we do with w.x.y.z?).

I would be hesitant to merge this in as it may have other consequences (i.e. regressions in version support)

I used the regexp from semvish.clean() exactly to prevent this. As for the sorting, I've already checked that it works just as well as semvish before using it for the API.

jimaek commented 8 years ago

I will merge it and deploy because we have projects not updating. If there is a better solution we can work on it while no projects are failing :)

megawac commented 8 years ago

what do we do with w.x.y.z?

That gets parsed as w.x.y-z. The only case that would be an issue is if the author also publishes w.x.y as w.x.y > w.x.y-z

MartinKolarik commented 8 years ago

@megawac https://tonicdev.com/56cc6014997fe20c0070f3a0/56cc6014997fe20c0070f3a1

megawac commented 8 years ago

Anyway, thats obviously a bug and would have been fixed if it had been reported. However, bot is parsing some versions incorrectly now. See https://github.com/jsdelivr/jsdelivr/pull/10143