Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Get organized: owner/repo = repo_spec #376

Merged
merged 2 commits into from
May 31, 2018
Merged

Get organized: owner/repo = repo_spec #376

merged 2 commits into from
May 31, 2018

Conversation

jennybc
Copy link
Member

@jennybc jennybc commented May 31, 2018

Main points:

  • Continue to standardize user-facing args and internal vocabulary around GitHub:
    • r-lib/usethis: owner = r-lib, repo = usethis, repo_spec = r-lib/usethis
  • Add more helpers around GitHub owner, repo, repo_spec

Questions:

  • Is it bad to use non-exported functions in default arg values? General question.
  • Should the browse_*() family switch over to repo_spec from package? Right now, target repo is driven by GitHub info found in URL field of DESCRIPTION. But maybe it should be driven by the local remote associated with GitHub? Or try one then the other?
    • Pro: everything would work for non-package projects
    • Con: less favourable if you're working with someone else's package. But how relevant is that?

Happy to discuss/review during 1-on-1.

@jennybc jennybc requested a review from hadley May 31, 2018 05:44
R/create.R Outdated
}
owner <- repo[[1]]
repo <- repo[[2]]
## TODO: do I need to do anything re: repo --> repo_spec?
Copy link
Member Author

@jennybc jennybc May 31, 2018

Choose a reason for hiding this comment

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

Do I need to do much/anything about changing repo argument to repo_spec? create_from_github() is mostly for interactive use, probably has few users, repo/repo_spec is the first argument (so often unnamed) and partial matching works in our favour.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think you need to

repo = NULL,
from = releases(owner = owner, repo = repo)[[1]],
use_tidy_thanks <- function(repo_spec = github_repo_spec(),
from = releases(repo_spec)[[1]],
Copy link
Member Author

@jennybc jennybc May 31, 2018

Choose a reason for hiding this comment

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

I dislike using unexported functions for the defaults. But also like having a sensible default and revealing it.

Copy link
Member

Choose a reason for hiding this comment

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

Should we add this as an argument to use_github_labels() too?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think we need to add repo_spec to use_github_labels(). I makes sense to me that it works on the active project.

I think use_tidy_thanks() is different and pretty unique because you are likely to use it while in another project, e.g. composing a post in the tidyverse.org project.

R/create.R Outdated
}
owner <- repo[[1]]
repo <- repo[[2]]
## TODO: do I need to do anything re: repo --> repo_spec?
Copy link
Member

Choose a reason for hiding this comment

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

I don't think you need to

repo = NULL,
from = releases(owner = owner, repo = repo)[[1]],
use_tidy_thanks <- function(repo_spec = github_repo_spec(),
from = releases(repo_spec)[[1]],
Copy link
Member

Choose a reason for hiding this comment

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

Should we add this as an argument to use_github_labels() too?

@jennybc
Copy link
Member Author

jennybc commented May 31, 2018

We've decided to leave the browse_*() family alone for now, but it remains on the radar for future change.

@jennybc jennybc merged commit c21b284 into master May 31, 2018
@jennybc jennybc deleted the owner-repo-repospec branch May 31, 2018 23:06
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants