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

Add repo shortcuts (github, bitbucket, gitlab) #9

Merged
merged 7 commits into from Oct 28, 2014
Merged

Conversation

iarna
Copy link
Contributor

@iarna iarna commented Oct 7, 2014

Adds a github prefixed shortcut and matching bitbucket and gitlab shortcuts. Adds detection and folding of github/bitbucket/gitlab git urls into shortcuts to allow consumers to do special handling without having to special case.

@iarna iarna force-pushed the add-other-repo-types branch 3 times, most recently from c386ba1 to 2f3402d Compare October 8, 2014 10:04
@iarna
Copy link
Contributor Author

iarna commented Oct 10, 2014

The commit history here could be folded up quite a bit really, as most of this work got pushed out into another module.

default:
throw new Error('Unsupported URL Type: ' + arg)
break
else if (/^git([+](https?|rsync|ftp|ssh|file))?:$/.test(urlparse.protocol) ||
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: Now that hosted git checking is totally separate, change this code back to what it was.

@zeke
Copy link

zeke commented Oct 11, 2014

Nice! Is there accompanying documentation that explains what this is and how to use it? I peeked through some of the commits and didn't see any markdown files.

@zeke
Copy link

zeke commented Oct 11, 2014

Also: It looks like the work is already done, but I'll just leave these here in case:

There's a GUI for the github one: https://github-url-to-object.herokuapp.com/

@iarna
Copy link
Contributor Author

iarna commented Oct 13, 2014

@zeke Oh, I wish I'd known those existed! The actual redundant code lives here: https://github.com/npm/hosted-git-info

Docs wise, thanks for catching that, I do want to add the other repo types to the README where github is already mentioned.

@iarna iarna force-pushed the add-other-repo-types branch 2 times, most recently from 112078b to 27c2b7d Compare October 16, 2014 20:38
@iarna
Copy link
Contributor Author

iarna commented Oct 16, 2014

Hmm, I'm actually thinking that this should dump the host specific prefixes (github/bitbucket/gitlab) and just have type=hosted and spec=hosted-git-info...

@zeke
Copy link

zeke commented Oct 17, 2014

@iarna what is the end-user purpose of this change? Forgive my ignorance, but I don't really know what npm-package-arg does. Is the goal to allow users to be more explicit when specifying dependencies in package.json? README-driven development can help inform design decisions, and would shut me up too. :)

Purpose aside, I notice some inconsistency in these names:

  • sshurl - The ssh URL for this git repo
  • https - The HTTPS URL for this git repo
  • directUrl - The URL for the package.json in this git repo

@iarna
Copy link
Contributor Author

iarna commented Oct 20, 2014

@zeke The change? Well, currently we parse foo/bar as git+ssh://git@github.com/foo/bar.git. We'd like to be less github specific and more generally support other hosting services, thus the introduction of github:foo/bar, bitbucket:foo/bar, gitlab:foo/bar. But ALSO, and perhaps more importantly, the multi-stage install would like to know how to fetch the package.json from hosted repos without first having to clone the repo. In order to do that we need to be able to parse not only shortcuts like foo/bar or github:foo/bar, but also full urls. As I wanted to isolate that parsing as much as possible to make adding additional hosts relatively painless, I put it in a separate module (thus hosted-git-info). This module then consumes that, thus the interface changes here, and then finally npm consumes this.

Regarding the naming, see updated patch.

@iarna iarna force-pushed the add-other-repo-types branch 2 times, most recently from 9540020 to 4c31e41 Compare October 20, 2014 07:33

module.exports = npa

var isWindows = process.platform === "win32" || global.FAKE_WINDOWS
var slashRe = isWindows ? /\\|\// : /\//
var slashRe = isWindows ? /\\|[/]/ : /[/]/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

\/[/] feels gratuitous, here and elsewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See below

@othiym23
Copy link
Contributor

A little more contextual documentation would be nice here. Aside from that, and the nits listed above, this looks good. Feel free to squash it down if you like, but keeping the functionality changes and the code cleanup changes in separate commits would be helpful. :shipit:

@iarna
Copy link
Contributor Author

iarna commented Oct 23, 2014

It's actually pretty squashed already, as there were previous design iterations. =p But yeah, I like keeping code cleanup and functionality changes distinct. And I think there's value in keeping module additions and their use distinct too. So yeah, no squashing.

@iarna
Copy link
Contributor Author

iarna commented Oct 23, 2014

I'm not clear, however, on what you're looking for regarding contextual documentation?

@iarna
Copy link
Contributor Author

iarna commented Oct 23, 2014

Oh, and the version bump, I should just mention, is major due to the dropping of the "github" type (as its replaced by type=hosted, hosted.type=github). Originally I just added gitlab and bitbucket types, but using a single hosted types lets us add more hosted git shortcuts by just editing hosted-git-info and bumping module versions. No npm code has to be touched, as it doesn't care what the "type" is, it just uses the urls in the hosted property.

@othiym23
Copy link
Contributor

I was going to ask for the major version bump if you didn't do it, so I'm with you there.

I guess I'd just like to see a little more verbiage around what the use cases are for this module and how it's supposed to be used. A little example? Especially now that you're adding more functionality into it, it would be helpful to give developers first encountering the module a little bit of orientation as to what it is and what it's for.

@iarna
Copy link
Contributor Author

iarna commented Oct 24, 2014

Ok, I changed improved the docs a bit, drawing on what I did with realize-package-specifier. Hopefully they'll feel a little more complete. With that, when it is again not the middle of the night, I'll merge this and propagate it down the chain. (This goes into a new realize-package-specifier which then goes into the multi-stage npm branch.)

@othiym23
Copy link
Contributor

Perfect! ⚓

@iarna iarna force-pushed the add-other-repo-types branch 3 times, most recently from cca65ad to 7b7c9f3 Compare October 28, 2014 04:36
@iarna iarna merged commit 263fd43 into master Oct 28, 2014
@iarna iarna removed the in progress label Oct 28, 2014
@iarna iarna deleted the add-other-repo-types branch October 28, 2014 09:09
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.

None yet

4 participants