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

Update simple_memo.rst #120

Closed
wants to merge 1 commit into from
Closed

Update simple_memo.rst #120

wants to merge 1 commit into from

Conversation

bwagner
Copy link

@bwagner bwagner commented Sep 21, 2020

Description

Please include a description of the change, and why it was needed.

Linked issues

Please link any issues this pull request related to. Using the word "closes" before the
link will mean the issue is automatically closed by this Pull Request.

Testing

Please describe how your changes have been tested.

Checklist

  • I have provided a good description of the change above
  • I have added any necessary tests
  • I have added all necessary type hints
  • I have checked my linting (docker-compose run --rm lint)
  • I have added/updated all necessary documentation
  • I have updated CHANGELOG.md, following the format from
    Keep a Changelog.

@bwagner bwagner closed this Sep 21, 2020
@bwagner
Copy link
Author

bwagner commented Sep 21, 2020

Giving up.

@jstockwin
Copy link
Owner

@bwagner No worries, git/github can take a while to get used to. It is a really cool think to learn though, so I'd recommend looking at the documentation and some tutorials. GitHub has some really good ones: https://docs.github.com/en/github/getting-started-with-github

I have just created #121 to fix the issue. Many thanks for pointing out the problem, and I hope this didn't cause you too much frustration!

@bwagner
Copy link
Author

bwagner commented Sep 22, 2020

Thank you for your suggestions, @jstockwin! I'm not surprised I come across as a newbie, even though I've been on github since 2010.

I hadn't understood (and still don't) whether you had actually made changes to my PR or whether you had simply suggested changing the links along with the prose amendments in my PR.

Had you done changes to my PR?

How would I have accepted them into my PR?

Also, after I gave up on my original PR, I made a new one with your suggested improvements. Why wasn't that PR used?

Thank you

@jstockwin
Copy link
Owner

@bwagner Apologies for assuming you were a newbie!

Let me answer your questions in a slightly different order to what you asked them in:

Also, after I gave up on my original PR, I made a new one with your suggested improvements. Why wasn't that PR used?

I saw your "Giving up" comment (here #120 (comment)) and figured something had gone wrong. I didn't even look at the changes, but I now agree the changes were correct.

Given your comment and closing the issue, I assumed your second attempt was just starting from scratch entirely and had still gone wrong somehow (which is why I assumed you were a newbie). I guess you didn't actually mean to comment and close that new PR (i.e. this PR)?

Had you done changes to my PR?

Not quite, I had left a review suggesting changes, it was still for you to accept them into your PR. (See point 6 of https://docs.github.com/en/github/collaborating-with-issues-and-pull-requests/commenting-on-a-pull-request#adding-line-comments-to-a-pull-request), which is what I did.

In fairness to you, the suggested changes thing is a fairly new feature.

How would I have accepted them into my PR?

There should have been a "Commit Suggestion" option which would have walked you through incorporating my change into your PR (as an additional commit), see https://docs.github.com/en/github/collaborating-with-issues-and-pull-requests/incorporating-feedback-in-your-pull-request#applying-suggested-changes.

I think what you did was "Mark as resolved", which kind of closes the conversation but does not actually do any changes. See https://docs.github.com/en/github/collaborating-with-issues-and-pull-requests/about-pull-request-reviews#resolving-conversations. It might be that once it's marked as resolved it is no longer possible to apply the suggestion, I am not sure...

Thanks again for submitting the issue, and sorry for the confusion and not ending up getting your PR merged.

@bwagner
Copy link
Author

bwagner commented Sep 22, 2020

Awesome, @jstockwin, thanks a lot for taking the time to explain. I appreciate it.

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