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

Support npm-style #ref suffix; fixes issue 16 #17

Closed
wants to merge 2 commits into from

Conversation

dankegel
Copy link

Also save one call to re.search().

@dankegel dankegel force-pushed the issue16-support-refs branch from 53a4b75 to 89e93af Compare May 24, 2018 19:12
@dankegel dankegel mentioned this pull request May 24, 2018
@dankegel
Copy link
Author

dankegel commented May 24, 2018

It's an open question whether the last commit should be included.

It's also an open question whether branch should default to None or to "master".

The code I'm writing that uses this now looks like

  parsedrepo = giturlparse.parse(repourl)
  name = parsedrepo.name
  # Don't know what giturlparse defaults will be for branch and href yet.
  # I want to write
  #  branch = parsedrepo.ref
  #  repourl = parsedrepo.href
  # but until those are nailed down, be more careful:
  branch = parsedrepo.ref if parsedrepo.ref else "master"
  repourl = repourl.split('#')[0]

@retr0h
Copy link
Collaborator

retr0h commented May 24, 2018

Why would we be parsing npm urls?

'ref': None,
},
# npm and others allow specifying a branch or commit. See
# https://docs.npmjs.com/files/package.json#git-urls-as-dependencies
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why would this ever be used? https://docs.npmjs.com/files/package.json#git-urls-as-dependencies? Thats an NPM url.

Copy link
Author

Choose a reason for hiding this comment

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

That's just an example of one project that uses the convention. yarn uses it, too.

I've been using it in my scripts to configure buildbot, and I find it useful.

Copy link
Author

Choose a reason for hiding this comment

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

That said, it's not that much code to do without this feature.

Copy link
Collaborator

@retr0h retr0h May 24, 2018

Choose a reason for hiding this comment

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

Sure, but npm isnt a git url neither are yarn urls. I have not seen a git url having this format so I would have a hard time accepting a PR like this bc it's outside the scope of the project.

Copy link
Author

Choose a reason for hiding this comment

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

I think it's a useful convention even outside of npm, but it's easy to implement as a tiny wrapper around git-url-parse, so closing.

@dankegel dankegel closed this May 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants