npm1k / npm1k.github.io

https://npm1k.org
Apache License 2.0
22 stars 4 forks source link

use `jonschlinkert/parse-github-url` instead #6

Closed tunnckoCore closed 9 years ago

tunnckoCore commented 9 years ago

it is most complete and the best solution for parsing maybe 99.9% of all or all weird use cases.

see the coverage https://github.com/jonschlinkert/parse-github-url

kemitchell commented 9 years ago

@tunnckoCore, did you notice any bad links or other errors due to github-url-to-object?

tunnckoCore commented 9 years ago

there have in the wild and also noticed that line which mention that git+http(s) urls cant be parsed by github-url-to-object, which is weird, cuz if you npm init and just write "foo/bar" for the repository input it will generate exactly that type of urls.

omg, so mistakes, im :sleepy:

kemitchell commented 9 years ago

Does normalize-package-data normalize all the relevant URLs?

beaugunderson commented 9 years ago

So I may have jumped the gun on the change I made to add the replacement (though it doesn't break anything); I tested via the demo site and that's where I ascertained that the git+ssh:// URLs didn't work--but I think that might just be a bug in the demo site and not the actual module!

kemitchell commented 9 years ago

Thanks, @beaugunderson!

kemitchell commented 9 years ago

And thanks to you, too, @tunnckoCore! Happy to receive your PR.

beaugunderson commented 9 years ago

For example, if you try this URL in the demo site linked above it fails to parse:

git+ssh://git@github.com:npm/npm.git

And it looks like it is legitimately broken, so the replace(/^git\+/, '') call is needed:

> g = require('github-url-to-object')
[Function]
> g('git+ssh://git@github.com:npm/npm.git')
null
beaugunderson commented 9 years ago

Hmm... Even replacing git+ in the test URL above fails because of the : later in the URL, so I think we should definitely merge this (I tested that it works with all the variations that fail above). :)

tunnckoCore commented 9 years ago

@kemitchell @beaugunderson that's why i made this pr and talking to use parse-github-url

see https://github.com/jonschlinkert/parse-github-url/blob/master/test.js