Hacker News Reader

Comment by Kwpolska | Hacker News Reader

CPython seems to use squash merges, which means only one commit will end up on the main branch after merging this PR. The history on branches is irrelevant and can be completely messy, full of merges and other experiments; the main branch has one commit per actual feature/change.

And eh, conventional commits seem like pointless bureaucracy to me.

View on HN

With only +1,722 lines added, even if the commits were eventually squashed upon landing, I'd consider it good etiquette to tidy up changes to maybe a handful of logical commits instead of pushing 404 raw commits.

Or maybe it's another weird pun on 404 Not Found? I can't tell by now...

The end result of doing this is good, but I find it really difficult to cleanly do this before I have something that's 100% complete.

I don't code linearly like "first I need feature A, then I code feature B which is needed for feature C, and so on"

It's usually a bit all over the place and it's not clear what depends on what until I start reaching the end.

So to do this properly I'd need to spend a day or two rewriting or making a new branch that cleanly adds everything in order. Hopefully in a way that doesn't leave master in a broken state when reverting tail commits.

In addition, when doing multiple pull requests for a single high level feature, you might get some comments about pull request "C" that would require changes in pull request "A"

How the hell is someone supposed to review your pull request if you don't take the time to clean it up?

I normally go through every single individual commit when reviewing something and find the commit messages extremely helpful to understand what some change is supposed to do.

Yes, cleaning up your commits takes some time butt I don't see an alternative if you don't work alone and want your code to stay maintainable.

I review the pull request as a whole, looking at the diff between main and the latest commit on the branch (i.e. what GitHub/etc show by default). Reading commit-by-commit means you’d read code that the author knows is wrong and had already fixed it, but you’re cluttering your mind with it. During re-reviews, I usually look at the diff between the last commit I reviewed and the newest commit.

> Reading commit-by-commit means you’d read code that the author knows is wrong and had already fixed it

If the commit is wrong, it shouldn't be there. I expect every commit in a Pull Request to be functional on its own or I am not going to approve it in the first place. Git has tools to rewrite your commit history and you should use them.

The whole point is that I should be able to revert individual commits without code breaking. At least that is the ideal. A clean version history matters a lot of the people maintaining your code down the line.

Yes and no.

I think what you say is definitely the goal for day-to-day contributions.

However, there are changes to a code base that are more "Manhattan project" in nature where not all changes can be neatly packaged into their own commits, OR the PR author kind of needs to re-do their coding on a clean room branch. Which is significant overhead.

Being able to undo a commit is a means to an end, not the ultimative goal.

In my experience, this is very team-specific. Some teams want squash merges and ignore individual commits and only look at the latest version, while others care about the "history" and will tidy it up in the PR and then merge all the commits from the PR. Though I've found the latter to be much more rare, that's why some tools (like Reviewable https://docs.reviewable.io/reviews.html?highlight=commit-by-...) have a commit-by-commit option but the default is to combine them for review.

> I find it really difficult to cleanly do this before I have something that's 100% complete

That's what a DVCS like git makes easy to do, it's really worth learning.

Yes, but for such a significant contribution to a huge project it's good etiquette to squash on your own before submitting the PR. (Not that it means the PR shouldn't be reviewed and accepted.)

Honestly, I frequently do this for my own personal projects since I'm lazy, but if I'm submitting something to a big open source project I always clean it up first.

Being honest though, the guy's commit messages influence my prior on how reliable and well-designed his code will be.

“My prior”. Give it a rest.

Just saying that if I were working with this person it wouldn't make me think highly of him, and in my fairly extensive experience I can report that there's a strong correlation between silly commit messages and not great code. I didn't mean to imply that I was qualified or skilled enough to evaluate the JIT compiler for Python.

I think I missed what your point was?

I understood the point to be about using accessible language rather than jargon.

You think "my prior" is jargon? I'm not sure what to say to you then. You realize Bayesian thinking in statistics has been around since the 1950s if not 1930s? https://en.wikipedia.org/wiki/History_of_statistics#Bayesian...

Bear in mind I'm interpreting the previous comment.

I'm familiar with many things Bayesian thanks :) However, I wouldn't assume everyone else is, even here.

Are you familiar with the meaning(s) of the word jargon? https://dictionary.cambridge.org/dictionary/english/jargon https://en.wikipedia.org/wiki/Jargon

Just to provide an example, your previous comment could have been written something like this: "Being honest though, the guy's commit messages changed my preconceptions about how reliable and well-designed his code will be."

No knowledge of statistics required, Bayesian or otherwise.

OK, fair enough, your suggestion is totally reasonable. However I've been referring to people's "priors" though in informal conversation for about 25 years, to friends, romantic partners, and family as well as academics and programmers, and I know several other people who do the same. Apart from anything else it's a nice non-technical sounding word. I'm not a Bayesian statistics zealot (I don't even work in statistics any longer). But I definitely think all educated people should be familiar with the _idea_ of Bayesian inference. I think that goes without saying. I'm no expert on such matters but clearly our own perception/cognition has some sort of Bayesian flavour to it (you think a mammal dimly perceived on the horizon is probably a dog etc). What I'm saying is -- it sound like perhaps you also have had some involvement with the academic subject -- I think you don't need to push that word quite so far away from mainsteam culture. It's perhaps even a little patronizing to mainstream culture? And I think that if we are ever going to overcome CP Snow's Two Cultures problem then making little gestures like this in the right direction is actually important; especially from people like you and me.

Reply on news.ycombinator.com