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

Clarify to not commit proposed change #904

Merged
merged 3 commits into from
May 3, 2023
Merged

Clarify to not commit proposed change #904

merged 3 commits into from
May 3, 2023

Conversation

lexnederbragt
Copy link
Contributor

A learner missed this and had already committed the change before testing git checkout. This change intends to help avoid this mistake.

A learner missed this and had already committed the change before testing `git checkout`. This change intends to help avoid this mistake.
@zkamvar zkamvar mentioned this pull request Nov 29, 2022
@kekoziar
Copy link
Contributor

kekoziar commented Dec 1, 2022

@lexnederbragt Having the challenge written in such a way that a learner never misses it, defeats the purpose of the challenge. Did they miss it because it's unclear, or because the learner didn't quite grasp the difference between a change being staged and committed? I'd rather not write in such a way that it takes into account each mistake a learner might make.

That said, this would probably be best clarified with a solution to the challenge. Are you willing to write up a good solution instead, taking into consideration the common mistakes learners might make with this?

@lexnederbragt
Copy link
Contributor Author

I see your point and agree that my change would be making the challenge less, well, challenging.
I like the idea of adding a solution and will submit one soon.

@lexnederbragt
Copy link
Contributor Author

PR updated with a proposed solution. Please review!

Copy link
Contributor

@kekoziar kekoziar left a comment

Choose a reason for hiding this comment

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

What version git are you running? My version is prompting with git restore not git checkout

@kekoziar kekoziar added the status:waiting for response Waiting for Contributor to respond to maintainers' comments or update PR label Apr 30, 2023
@lexnederbragt
Copy link
Contributor Author

I agree that recent versions of git use git restore to restore files instead of git checkout. However, the lesson is based on older versions of git, and uses git checkout. So, this PR needs to use the current lessons convention.

See also discussion in #691 on updating the git lesson to version of git that uses git restore (and git switch).

… add` in challenge prompt.

Update 05-history.md
Copy link
Contributor

@kekoziar kekoziar left a comment

Choose a reason for hiding this comment

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

Looks good. I made some minor changes to make the input/output in the solution standardized with the rest of the lesson, and added clarification in the challenge prompt.

Thank you for submitting this solution!

@kekoziar kekoziar merged commit e5f724c into swcarpentry:gh-pages May 3, 2023
@kekoziar kekoziar removed the status:waiting for response Waiting for Contributor to respond to maintainers' comments or update PR label May 3, 2023
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.

2 participants