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

Issue #507 - Episode 14 - Added guidance for RStudio users when Git.exe path not pre-filled #531

Merged
merged 11 commits into from
Jan 26, 2019

Conversation

richmccue
Copy link
Contributor

In response to Issue #507 I added some text to Episode 14 to give some direction to users when the Git executable path in RStudio is not pre-filled for them. This seems to be more of a problem on Windows computers.

@murraycadzow
Copy link

This looks like a really good addition.
It might be worth rephrasing the sentence about making sure that git is installed. A learner might have git installed as part of the OS and it's not being found and didn't actually download it themselves as part of the setup. Maybe have the urls as a suggestion if it isn't installed.

@munkm
Copy link
Contributor

munkm commented Sep 19, 2018

For transparency: I requested review from some of the maintainers from the r lessons that regularly use git with RStudio to weigh in on this PR.

This PR looks great to me @richmccue . Would you mind updating your PR with @murraycadzow 's suggestion and then I'll merge it into the lesson. The clarifying work you've done here is really helpful and I think it will be appreciated by all of the learners interested in git+RStudio.

@munkm munkm added status:in progress Contributor working on issue status:changes requested Waiting for Contributor to update PR type:clarification Suggest change for make lesson clearer labels Sep 19, 2018
@diyadas
Copy link

diyadas commented Oct 3, 2018

Looks good to me!

@munkm munkm self-requested a review January 21, 2019 10:13
Copy link
Contributor

@munkm munkm left a comment

Choose a reason for hiding this comment

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

I'm happy to merge if the slight clarification mentioned in the discussion is incorporated. Alternatively we can open an issue for these changes if @richmccue isn't able to update the PR and merge as is. Let us know what you think richmccue!

@katrinleinweber
Copy link
Contributor

katrinleinweber commented Jan 21, 2019

Due to carpentries/workshop-template#574 I have a fresh macOS available that hasn't seen RStudio now. My approving review was from before that. Shall I test the instructions?

@munkm
Copy link
Contributor

munkm commented Jan 21, 2019

Sure that sounds good @katrinleinweber.

I've updated the language around making sure that git is installed as suggested by @murraycadozow (Issue swcarpentry#507) to take into account the face that Git may be installed as part of the OS
@richmccue
Copy link
Contributor Author

I've updated the language around making sure that Git is installed as suggested by @murraycadozow (Issue #507) to take into account the fact that Git may be installed as part of the OS.

@munkm
Copy link
Contributor

munkm commented Jan 23, 2019

Awesome! Thank you so much @richmccue. I'm happy to merge, but I'd like to wait until @katrinleinweber verifies that they can follow the instructions on their machine to double check everything works. 🙂

> Copy the path to the git executable (e.g. On one Windows computer which had
> GitHub Desktop installed on it, the path was:
>
> If there is no version of Git on your computer, please either install [Git](https://git-scm.com/downloads/)
Copy link
Contributor

Choose a reason for hiding this comment

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

How about referring to https://carpentries.github.io/workshop-template/#git here so that we do not need to maintain extra instructions here at all? https://swcarpentry.github.io/git-novice/setup.html is already doing that as well.

Copy link
Contributor

@munkm munkm Jan 25, 2019

Choose a reason for hiding this comment

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

Alternatively if the directions are clear we can merge this PR and open an issue for another new contributor to update this URL. richmccue has already spent a lot of time on this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Which can not be recovered unfortunately, and therefore should not be part of the decision when what gets merged ;-)

I don't mean this harshly at all; Please don't get me wrong! I'm grateful to @richmccue for alerting us to the problem and for providing a fix :-) Just pointing out that previous maintainers choose to refer to the setup info that's maintained centrally.

Will test this on the weekend and report back.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for the helpful wikipedia link.

I agree that this link should point to the same instructions as the setup to be consistent with the rest of the lesson. I do not think that that merging this PR should be blocked by this link being inconsistent.

@katrinleinweber
Copy link
Contributor

Reading from the top again, I can't help after all. This "seems to be more of a problem on Windows computers." Presumably, "learner might have git installed as part of the OS" prompted me to to comment upon reading it initially, sorry.

@munkm
Copy link
Contributor

munkm commented Jan 26, 2019

Thank you so much for all of your hard work @richmccue!

@munkm munkm merged commit bc5979d into swcarpentry:gh-pages Jan 26, 2019
@kekoziar kekoziar removed status:changes requested Waiting for Contributor to update PR status:in progress Contributor working on issue labels Jul 3, 2019
zkamvar pushed a commit that referenced this pull request May 8, 2023
Issue #507 - Episode 14 - Added guidance for RStudio users when Git.exe path not pre-filled
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:clarification Suggest change for make lesson clearer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants