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

Parse GitHub URLs #112

Merged
merged 11 commits into from Nov 14, 2017
Merged

Parse GitHub URLs #112

merged 11 commits into from Nov 14, 2017

Conversation

@jennybc
Copy link
Member

@jennybc jennybc commented Sep 20, 2017

The promised PR that allows install_github() to accept a variety of GitHub URLs. It should be merged after #110, because this branch works off that base. Or, I guess, I could close that, because those commits are included here as well.

I took the liberty of doing a general tidy of GitHub remote parsing:

  • Split GitHub repo parsing into two exported functions: parse_repo_spec() (the thing we've always had) and parse_github_url(). They are documented together.
  • Moved the dropping of empty elements into the (unexported) parse_git_repo() to make the return value of the above functions more predictable.
  • Put all related tests in their own file because it's nice to be able to do test(filter = "parse") w/o doing the full install_github() tests.

I know it still needs a NEWS bullet. I'll write one with correct issue/PR numbers if this is good to merge.

@jennybc jennybc requested a review from gaborcsardi Sep 20, 2017
@jennybc
Copy link
Member Author

@jennybc jennybc commented Sep 21, 2017

To really make installation via URL work for, say, GHE remotes, the parsed list would have to gain a slot to hold the host 🤔.

Loading

NAMESPACE Outdated
@@ -35,7 +35,8 @@ export(install_url)
export(install_version)
export(local_package_deps)
export(package_deps)
export(parse_github_repo_spec)
export(parse_github_url)
export(parse_repo_spec)
Copy link
Contributor

@gaborcsardi gaborcsardi Nov 12, 2017

Choose a reason for hiding this comment

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

It is an incompatible change to drop parse_github_repo_spec, I think we should keep that name (as well?).

Loading

Copy link
Member Author

@jennybc jennybc Nov 12, 2017

Choose a reason for hiding this comment

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

OK I will address this. Need to re-orient myself to what I was doing!

Just for context, I can't find any evidence of parse_github_repo_spec() usage on CRAN, outside of remotes itself.

GitHub-wise, I see one other use:

Loading

Copy link
Contributor

@gaborcsardi gaborcsardi Nov 12, 2017

Choose a reason for hiding this comment

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

Thanks!

Loading

Choose a reason for hiding this comment

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

As requested here: #13 😀

Loading

Copy link
Member Author

@jennybc jennybc Nov 12, 2017

Choose a reason for hiding this comment

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

OK I went back to the old name, @gaborcsardi.

Here's what I think (?) I was thinking:

  • Start to de-emphasize the GitHub-ness, since these repo spec parsers are not too hard to generalize to BitBucket, GitLab, and the like. But it's easy to make this change now and cross that bridge later.

Loading

@gaborcsardi
Copy link
Contributor

@gaborcsardi gaborcsardi commented Nov 13, 2017

Thanks! Can we just make parse_github_repo_spec an alias to the new function? Then we don't need two implementations. I understand that the new function is doing more, but that is fine.

Loading

params$release <- "*release"
}

params[grepl("^[^\\.]", names(params))]
}

parse_git_repo <- function(repo) {
Copy link
Member Author

@jennybc jennybc Nov 13, 2017

Choose a reason for hiding this comment

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

Should this be exported?

Loading

Copy link
Contributor

@gaborcsardi gaborcsardi Nov 13, 2017

Choose a reason for hiding this comment

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

Up to you. Personally I would not export any parsing functions, but we are past that already...

Loading

Copy link
Member Author

@jennybc jennybc Nov 13, 2017

Choose a reason for hiding this comment

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

OK let's leave it alone for now then.

Loading

@gaborcsardi
Copy link
Contributor

@gaborcsardi gaborcsardi commented Nov 14, 2017

Great, thanks!

Loading

@gaborcsardi gaborcsardi merged commit 2363c85 into master Nov 14, 2017
4 checks passed
Loading
jimhester added a commit to jimhester/pkgdepends that referenced this issue Nov 28, 2017
A port of @jennybc's r-lib/remotes#112 to
pkgdepends.
jimhester added a commit to jimhester/pkgdepends that referenced this issue Nov 28, 2017
A port of @jennybc's r-lib/remotes#112 to
pkgdepends.
jimhester added a commit to jimhester/pkgdepends that referenced this issue Nov 28, 2017
A port of @jennybc's r-lib/remotes#112 to
pkgdepends.
jimhester added a commit to jimhester/pkgdepends that referenced this issue Dec 4, 2017
A port of @jennybc's r-lib/remotes#112 to
pkgdepends.
gaborcsardi added a commit to r-lib/pkgdepends that referenced this issue Dec 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants