CodeNewbie Community 🌱

Cover image for Pull requests to branch or commit? Am I doing it right?
Robert Andrews
Robert Andrews

Posted on

Pull requests to branch or commit? Am I doing it right?

I've been trying to learn a bit about proper developer practice with git/GitHub etc.

Today, I made my first pull request! 👍🏻 But I need some clarification...

It went like this

I had already forked the repo owner's code, a WordPress theme...

GitHub logo robertandrews / blankslate

This is the official and original BlankSlate HTML5 WordPress theme boilerplate most commonly known to designers and developers. Released under the GPL.

=== BlankSlate ===
Contributors: tidythemes, bhadaway
Donate link: https://calmestghost.com/donate
Theme link: https://github.com/tidythemes/blankslate
Tags: accessibility-ready, one-column, two-columns, custom-menu, featured-images, microformats, sticky-post, threaded-comments, translation-ready
Requires at least: 5.0
Tested up to: 5.8
Stable tag: trunk
License: GNU General Public License v3 or Later
License URI: https://www.gnu.org/licenses/gpl.html
Please read: tidythemes.com/concept
== Description ==

IMPORTANT — PLEASE READ: tidythemes.com/concept

YOU MAY DELETE THIS FILE AND ANY OTHER FILE(S) BEFORE STARTING YOUR PROJECT

BlankSlate is the definitive WordPress boilerplate starter theme. We've carefully constructed the most clean and minimalist theme possible for designers and developers to use as a base to build websites for clients or to build completely custom themes from scratch. Clean, simple, unstyled, semi-minified, unformatted, and valid code, SEO-friendly, jQuery-enabled, no programmer comments, standardized and as white label as possible, and most importantly, the CSS is reset for cross-browser-compatability and no intrusive visual CSS styles have been added whatsoever. A perfect skeleton

I saw some HTML validation errors, and filed three issues on his repo.

https://github.com/tidythemes/blankslate/issues/32
https://github.com/tidythemes/blankslate/issues/33
https://github.com/tidythemes/blankslate/issues/34

Then I thought, "hey, I'll start to fix these right now". So I jumped on the first issue and fixed it locally in VS Code. Then, I pushed it to my forked repo...

I guess that was only in my repo until I did a pull request. I can't quite remember how I did the pull request, but it was in the GitHub web interface, as opposed to in VS Code.

I wrote a PR message referencing the number for the corresponding issue, filed the request and felt pretty proud of myself 🥳.

Now for where I'm uncertain

What if I also want to fix the other two issues and contribute them back?

I fixed a second one locally, committed that and pushed it to my GitHub forked repo.

But, to my surprise, this

a) didn't stay in my report, it went straight as a PR to the original repo owner.

b) specifically, it seems to go against the first pull request. I didn't seem to get the opportunity to make a new pull request 🤷🏻‍♂️.

That feels bad because the second commit shouldn't really refer to the issue referenced by the first issue/first pull request; it should reference the second issue.

I tried aimlessly to use some Revert options inside VS Code, then re-push. That seemed to work, but it still kept going against that first pull request.

Image description

In fact, there are a total of 6 commits against that pull request, many of which are just the back-and-forth of reverting and re-pushing. It feels embarrassingly untidy 🤦🏻‍♂️.

Image description

So, what do I do?

Why is this happening?

Should I deduce (learn) that pull requests are filed against branches, not against commits... ?

Rules of commitment: ie. Will all commits I make against against my master branch now also go against my PR in the original developer's repo? Even ones I may want to push for myself alone?

Separate fixes: If I want to create multiple pull requests to contribute back individual fixes, should I be making a new branch for each? (We are talking minor fixes here - like, just adding the odd tag).

Or should I be making a single, all-encompassing pull request and making three commits against that?

Basically, I had assumed that I could create pull requests against individual commits.

What's the practice here?

What do I need to learn?

Also

I'm also a bit confused about the order in which to do things...

  • Is it necessary/form to create an issue, and then a corresponding PR, or is this overkill?

  • Do I need to do a commit-push, and then a corresponding pull request? Or does a straight-up pull request actually end up creating that commit for me, too?

Latest comments (2)

Collapse
 
terabytetiger profile image
Tyler V. (he/him)

I'll try to address everything - if I miss any specific questions let me know and I can try to answer those as well :)

  • The Pull Request is a request to merge your branch into the target branch (tidythemes:master here). Until the PR is merged, future commits to your branch will be included in the PR. This way if the repository owner wants any adjustments to the branch before merging, you can make changes without having to open a new PR.

  • Many open source projects will include a CONTRIBUTING.md file which outlines their preferred process for submitting PRs. It may include opening an issue first, it may not. In my experience it's fine to just create a PR if there is no issue - and open an issue if you aren't planning to fix it (again - my experience, some repository owners may have alternative preferences).

  • The above also addresses the "One big PR or 3 small PRs" question, but if it's not outlined in the contributor guidelines, it's up to you to determine if the issues are related or small enough to roll into a single PR or split into separate requests.

  • You do need to commit -> push -> create PR. Commits are like "save points" and if you never commit and push, the branch won't have any new save points to compare back to the original source.

I think your experience is a common one - git isn't particularly user friendly at first 😅 And it feels particularly insulting when it creates the commits for reverting commits (imo).

Collapse
 
robertandrews profile image
Robert Andrews

Thanks Tyler.