-
Notifications
You must be signed in to change notification settings - Fork 50
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
Versions returned in the wrong order #26
Versions returned in the wrong order #26
Conversation
Once this is approved I can squash into one commit. |
@@ -159,7 +159,7 @@ server.get('/:name', function (req, res) { | |||
var meta = data.packageMeta(packageName); | |||
if (!meta) return notFound(res); | |||
|
|||
var versions = data.whichVersions(packageName).sort(); | |||
var versions = data.whichVersions(packageName).sort(semver.compare); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we hide sorting inside whichVersions
and rename the function to whiteVersionsSorted
? This should reduce the amount of repeated code (see L213 for another copy).
The pull request looks quite good, I appreciate that you included unit-tests for your change. I left few comments above (in code). |
} | ||
}; | ||
} | ||
} | ||
|
||
function getPackageVersions(packageName) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just DRYed two birds with one stone by adding this. Hopefully that is an acceptable alternative to altering data.js.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, your solution is fine.
Hi @mike-spainhower, I noticed there was no activity in your pull request for almost a month. Is there a way I can help you? We need to address the issues mentioned in this comment to get the patch merged. |
Truth be told, I got halfway through fixing those comments and then got a CouchDB registry going. I still intend to get these resolved, but I have to do it in spare time now 🐼 |
Hi, I'm experiencing the same bug. So my users can't install the latest version of packages. |
@evil-shrike If you want to PR my branch to address @bajtos code review comments, we could get this through. Otherwise I'll still have to find some free time to rebase and DRY up tests. |
@mike-spainhower How can I help? |
I'd say you can fork node-reggie repo, pull changes from this pull request and address items 1,3,4,5 from my comment. Let's ignore the item 2 (file naming) for now. Rebase the changes on the current master branch and make sure the tests are passing on Travis CI.
That would make future changes more difficult to make. If there is nobody having enough time to improve the small amount of new code now, I don't think there will be anybody having even more time to improve all that not-good-enough code accumulated over time. |
Closes #25