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

Broken with \n in title #493

Closed
wants to merge 21 commits into from
Closed

Broken with \n in title #493

wants to merge 21 commits into from

Conversation

muschellij2
Copy link
Contributor

@muschellij2 muschellij2 commented Nov 13, 2018

It looks as though 65764fa#diff-4ac879304421b0c163d57d6d4be6b43dL89 changed the gsub part of the pkg$Title for repo_desc in use_github, which causes gh::gh to fail if there is a newline \n.

This should revert that behavior.

Copy link
Member

@jennybc jennybc left a comment

Would you be willing to branch off current usethis master and make a new PR? I think the semi-complicated history of master branch in your fork is making the optics of this PR more challenging.

@@ -28,6 +28,9 @@ use_git <- function(message = "Initial commit") {
if (yep(commit_consent_msg)) {
done("Adding files and committing")
git2r::add(r, paths)
if (length(paths) > 0) {
Copy link
Member

@jennybc jennybc Nov 13, 2018

Choose a reason for hiding this comment

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

I feel git_uncommitted() (in the condition for this block) implies that paths has non-zero length. Have you seen something to the contrary?

Copy link
Contributor Author

@muschellij2 muschellij2 Nov 13, 2018

Choose a reason for hiding this comment

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

This PR part was for #128

Copy link
Member

@jennybc jennybc Nov 13, 2018

Choose a reason for hiding this comment

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

If there are two different issues, let's split into two PRs then.

Copy link
Contributor Author

@muschellij2 muschellij2 Nov 13, 2018

Choose a reason for hiding this comment

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

LIttle swamped now, will within next week or 2.

Copy link
Member

@jennybc jennybc Nov 13, 2018

Choose a reason for hiding this comment

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

OK thanks!

@hadley
Copy link
Member

@hadley hadley commented Nov 25, 2018

Closing in favour of two smaller PRs

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

3 participants