Skip to content

Conversation

@stleary
Copy link
Owner

@stleary stleary commented Oct 14, 2015

Completed unit tests successfully. Leaving this pull request open for comment.for a few days.

@johnjaylward
Copy link
Contributor

Not to be a jerk, but the way you are merging my changes loses the history that shows me as the committer. If you need to create personal branches to test pull requests that's fine, but you should merge the original pull request once approved to keep the proper committer history.

@stleary
Copy link
Owner Author

stleary commented Oct 14, 2015

Actually I was just thinking about that and was considering posting a comment. Agreed, credit should be given to the author wherever possible. If nothing else, this helps link commits to the associated discussion. Briefly, these are the issues that come to mind that should be addressed if you want your own changes to be merged:

  • Any proposed change needs to pass unit tests, which I do by copying the changes locally. Then I push to a JSON-java branch exactly the changes that were tested. If I made a mistake in the transfer, at least I can be sure that the committed changes were tested. There is probably a clever way to do this with Git, but I have not hit upon it yet. Suggestions are welcome.
  • No one remembers to update the version date, so I usually have to fix that. Sometimes other changes are needed (e.g. update the README), but I don't think I have permissions to change someone else's branch. Maybe I could open a pull request on their branch?

@johnjaylward
Copy link
Contributor

You should be able to merge any pull request into any branch locally: https://help.github.com/articles/checking-out-pull-requests-locally/

it seems straight forward, but I haven't tried it myself.

For the file dates/readme, you could just mention it in the PR comments, or submit a PR onto their repository branch. essentially you'd fork them, checkout their PR branch, and commit, then submit a PR to them. That seems like a lot more work than just having them push a new version into their PR though.

I think since you are now using the github release section, it's not a bad thing to modify the README or file versions outside of the initial PR. It could even be done as part of the release process instead of as the commit process.

@johnjaylward
Copy link
Contributor

Looking close at the article I linked, that should do exactly what you want. It will give you all the commits from the original committer, than on top of that you can update the README or file versions in your own commit. You then push to a new branch like you've been doing and create a new PR off of your branch. The main difference is that the history is maintained.

@stleary
Copy link
Owner Author

stleary commented Oct 14, 2015

As an exercise, will work through the steps of using the original pull request. If successful, the changes will be merged on that branch.

@stleary stleary closed this Oct 14, 2015
@stleary stleary reopened this Oct 14, 2015
@stleary stleary closed this Oct 14, 2015
@stleary stleary deleted the jsonexception-inheritance-fix branch December 6, 2015 02:14
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.

3 participants