I would’ve rejected the PR too, but not for violation of that rule, but because one-line changes that merely fix a comment waste everyone’s time reviewing it, and are often just to build someone’s resume.
That’s exactly what I was talking about. You’re taking what they said reasonably, because you’re probably a reasonable person! However, look at what they’re actually saying. The issue wasn’t framed as being a “drive-by,” though later that’s what they claimed. It was about ideology. It was about politics. They didn’t pull up rules about one-line changes to justify not accepting them, they pulled up rules about talking politics.
The problem wasn’t that it was a meaningless PR, the problem was that it was a meaningful PR that they disagreed with.
And, quite frankly, disagreeing with that does make you an asshole, at the very least, and a transphobic misogynist, at worst. There were at least a few PRs open about similar issues, too.
Look, I’m not calling him a transphobe or a misogynist; I’m just saying this was an asshole thing to do, and it was done in an asshole way, and that allowing this sort of thing to exist, especially in FOSS, is not good. That’s all.
The issue wasn’t framed as being a “drive-by,” though later that’s what they claimed. It was about ideology.
But that’s the problem, it’s both a drive-by, useless change and a politically motivated one. If you show up to a project and submit a change that violates multiple rules, it’s dealer’s choice which one to pick.
With asynchronous discussions like this, it’s impossible to know their motivations, so it’s helpful to assume the best instead of the worst.
In order to not look like I’m just repeating myself over and over, here is another pull request where a user fixed the specifically gendered language, and was denied
Here’s the PR in question. It was merged, probably because it didn’t just change “he” to “they” in one spot (but did just that in a few spots), but actually fixed confusing language.
And then after it was merged, there were tons of irrelevant comments about the policy and other PRs.
The one I pulled here included changes from the other rejected PRs. Maybe this was by a different reviewer, idk. That said, it’s still a little iffy since it’s just fixing grammar and especially pronouns that aren’t really relevant to the code it’s commenting.
I probably would’ve accepted that last one because it fixes stuff in a lot of places rather than one (quantity has a quality of its own), and accepting it will hopefully stop PR spam.
Look, I’m not calling him a transphobe or a misogynist
He may be. Idk.
My criticisms here go to everyone involved:
reviewer should’ve rejected the PRs because they’re noisy, not because they’re “political”
submitter shouldn’t just submit a 1-line grammar fix in a comment
github users shouldn’t brigade, discussion should be technical
blog author should be more accurate (see above)
It’s stupid drama all around.
Fixing comments is fine. If you’re going to only fix comments, at least fix a bunch of them at once, and ideally more than just a pronoun or grammar mistake here and there. English isn’t everyone’s first language, so assume the best and don’t waste everyone’s time with useless changes.
It was. Some other member of SerenityOS, not the person behind Ladybird (awesomekling).
blog author should be more accurate (see above)
That’s fair. I’ll say though, the blog post is dated from 1 day after the PR was actually merged. It’s not unreasonable to think that, when they wrote it, it really hadn’t been merged and they only saw the initial denial citing the policy.
He may be. Idk.
Yeah, I was just trying to say that that wasn’t the point of my rant. I get it I get it.
It’s not unreasonable to think that, when they wrote it, it really hadn’t been merged and they only saw the initial denial citing the policy.
That never happened on this PR. The only human reply before the merge (aside from the submitter) was this:
Please fix the commit messages (see BuggieBot’s comment); and maybe this can go in one commit? Doesn’t really need to be 5 separate ones.
And this is BuggieBot’s comment:
Hello!
One or more of the commit messages in this PR do not match the SerenityOS code submission policy, please check the lint_commits CI job for more details on which commits were flagged and why.
Please do not close this PR and open another, instead modify your commit message(s) with git commit --amend and force push those changes to update this PR.
It’s a completely different.
This, plus the tone of the blog post looks like they were on a crusade instead of trying to accurately portray events.
Sorry to beat a dead horse here, my point is that we all need to be careful jumping to conclusions, especially in FOSS where discussion almost exclusively happens asynchronously in text and with people with different backgrounds. Pretty much everyone involved failed at that.
Right, but the policy was commit hygiene (lots of small commits), which has nothing to do with the “no politics” policy. It’s right there in the comment, and the suggestion is to squash the commits into one.
It’s alright. I think these discussions need to be had.
Agreed. And unfortunately, I felt it necessary to be really wordy to not come off as supporting intolerance in any way, while still arguing that I would’ve done the same (reject 1-line cosmetic PRs).
Right, but the policy was commit hygiene (lots of small commits), which has nothing to do with the “no politics” policy. It’s right there in the comment, and the suggestion is to squash the commits into one.
That’s exactly what I was talking about. You’re taking what they said reasonably, because you’re probably a reasonable person! However, look at what they’re actually saying. The issue wasn’t framed as being a “drive-by,” though later that’s what they claimed. It was about ideology. It was about politics. They didn’t pull up rules about one-line changes to justify not accepting them, they pulled up rules about talking politics.
The problem wasn’t that it was a meaningless PR, the problem was that it was a meaningful PR that they disagreed with.
And, quite frankly, disagreeing with that does make you an asshole, at the very least, and a transphobic misogynist, at worst. There were at least a few PRs open about similar issues, too.
Look, I’m not calling him a transphobe or a misogynist; I’m just saying this was an asshole thing to do, and it was done in an asshole way, and that allowing this sort of thing to exist, especially in FOSS, is not good. That’s all.
Check this out: https://mkultra.monster/tech/2024/07/03/serenityos-and-ladybird
But that’s the problem, it’s both a drive-by, useless change and a politically motivated one. If you show up to a project and submit a change that violates multiple rules, it’s dealer’s choice which one to pick.
With asynchronous discussions like this, it’s impossible to know their motivations, so it’s helpful to assume the best instead of the worst.
From that:
Here’s the PR in question. It was merged, probably because it didn’t just change “he” to “they” in one spot (but did just that in a few spots), but actually fixed confusing language.
And then after it was merged, there were tons of irrelevant comments about the policy and other PRs.
The one I pulled here included changes from the other rejected PRs. Maybe this was by a different reviewer, idk. That said, it’s still a little iffy since it’s just fixing grammar and especially pronouns that aren’t really relevant to the code it’s commenting.
I probably would’ve accepted that last one because it fixes stuff in a lot of places rather than one (quantity has a quality of its own), and accepting it will hopefully stop PR spam.
He may be. Idk.
My criticisms here go to everyone involved:
It’s stupid drama all around.
Fixing comments is fine. If you’re going to only fix comments, at least fix a bunch of them at once, and ideally more than just a pronoun or grammar mistake here and there. English isn’t everyone’s first language, so assume the best and don’t waste everyone’s time with useless changes.
Sigh, you do have a point.
It was. Some other member of SerenityOS, not the person behind Ladybird (awesomekling).
That’s fair. I’ll say though, the blog post is dated from 1 day after the PR was actually merged. It’s not unreasonable to think that, when they wrote it, it really hadn’t been merged and they only saw the initial denial citing the policy.
Yeah, I was just trying to say that that wasn’t the point of my rant. I get it I get it.
That never happened on this PR. The only human reply before the merge (aside from the submitter) was this:
And this is BuggieBot’s comment:
It’s a completely different.
This, plus the tone of the blog post looks like they were on a crusade instead of trying to accurately portray events.
Sorry to beat a dead horse here, my point is that we all need to be careful jumping to conclusions, especially in FOSS where discussion almost exclusively happens asynchronously in text and with people with different backgrounds. Pretty much everyone involved failed at that.
I agree with the rest.
Yeah I was referencing that comment.
Sequence of events:
Precocious, certainly, and I agree it was misguided. The blog post was indeed emotionally motivated, that’s more than clear.
It’s alright. I think these discussions need to be had.
Right, but the policy was commit hygiene (lots of small commits), which has nothing to do with the “no politics” policy. It’s right there in the comment, and the suggestion is to squash the commits into one.
Agreed. And unfortunately, I felt it necessary to be really wordy to not come off as supporting intolerance in any way, while still arguing that I would’ve done the same (reject 1-line cosmetic PRs).
This is some kind of correlary to Poe’s Law, or perhaps Godwin’s Law.
Suspiciously close to what Hitler would say… /s
Lol, got me there.
Hitler would probably be a formatting Nazi too. Can’t have that <insert slur> extra whitespace.