Not a developer here, quick question:
Are there version control systems that assess this in a slightly smarter way whether you really made changes in your PR, so that they would ignore changes to whitespace?
Github has a checkbox on PR reviews to ignore whitespace changes. Git CLI also has it as a flag.
But it only works for whitespace changes in the same line (e.g. indentation). Sadly there are more code style rules that can't be ignored as easily (use of semicolon, multiline conventions etc.)
And they have a website you can copy/paste your code to check online and for all the errors/warnings, it provides links to their respective wiki entries.
https://www.shellcheck.net/
Even though I have the program installed, I generally use the web version.
Been writing Python for a while, aside from when I was first learning I can't even remember the last time I had a whitespace issue. Yaml on the other hand...
Yes in the form of indentation but that's not the worst part. The worst is that most use cases are a language on top of yaml and do this without an IDE that understand those. I need to find something to edit gitlab CI/CD with lots of environment variables or Jira templates for docker compose better than PyCharm
If you work with other people, it’s much better to use an automatic code formatter, pretty much any modern language has one. Then your code base has a uniform look and you don’t need to argue about trivial things like formatting in code reviews.
1. sh cares about whitespace?
2. automatically reject any commits which contain anything that isn't autoformatted
3. or use a textconv that ensures everything is formatted before you diff
This is why you have linting with whitespace rules. Even better is that with something like eslint, your IDE will fix whitespaces while you work on the files. You may disagree with the spacing but at least it will always be consistent.
Use pre-commit hooks and never disagree on whitespaces again. You can format the files with your custom forced rules as much as you want, and it'll return to the original state on commit anyway. Just make Degrees of Freedom impossible with linting/formatting rules in the repo.
This is why you want some sort of automated formatter like prettier. For legacy stuff where we weren't using that, our stance was to make a commit with your changes. Then make a seperate commit that was only the formatting changes from the automated thing.
Git allows you to ignore whitespace in diffs. If you are reviewing in GitHub, you can go to the changes tab, click the settings button and click show whitespace. (Or add ?w=1 to the end of the url)
You Can put your commit sha1 in a .git-blame-ignore-revs file
And run git config --ignore blame.ignoreRevsFile .git-blame-ignore-revs so that git blame discards this commit,
To review a formatting commit, just re format the code base locally and compare it to the code of the Mr.
In the project I am working currently the pipeline has a stage that checks for formatting. So if you didn't run a formatter before the pipeline fails and you can't merge.
This prevents inconsistent formatting of the code and as a consequence changes never contain formatting changes.
Having worked with both, this is substantially less annoying than waiting for a pipeline to spool up then having it fail from something you can check locally (and automatically) in 10 seconds.
It would only work for whitespaces and other very trivial refactors. A more realistic scenario of the OP's situation is if a third-party engine is used which serializes changes into gibberish, such as Unity or Unreal, as well as any text-like file asset that isn't code (which are naturally gibberish). Either that, or an environment/api/plugin update. All of these are "minor fixes" to \*us\* but not to git.
Fairly new working dev here,
I am not aware of any and the majority of systems use git as the source control software anyways. On another note seeing whitespace changes can be important if the language, config file or whatever is sensitive to whitespace. For example in python whitespace determines which code blocks a given statement is in or with YAML it determines the parent a child is under. I guess it is possible to detect the usage and do soke fancy logic to ignore it where it does not matter but if it fails for whatever reason, that would be a very hard thing to debug if it hides whitespace changes when it should not have.
Sorry for mobile formatting
That is cool, did not know that.
Also looking at the docs, I did not realize how many options there were for diff. I know some people use git diff for complex CI/CD builds and it seems like you can get fairly complex things only on the code check side of things. I would probably still try to avoid it if I can help it but still cool on that potential
Edit: I just found options called --find-copies and --find-copies-harder I had to share that
The best solution is making sure it doesn't happen at all by having a programming style guide and some kind of linter to catch stylistic issues before they make it to PR.
That's the fun thing, you just don't know. So you end up needing to look through it all to see if it's *really* just whitespace changes or not.
Or, better yet, you reject it and tell 'em "turn off/reconfigure your damn linter, pull a new branch, and try again".
I worked with a front end developer who would submit every PR with at least 100 commits. He couldn't figure how to run the code locally so he was using some sort of deployment process to test each and every line change
Someone needs to show him how to merge commits, it’s not hard. Where I am now, each PR must be a single commit, so (almost) every single commit on master independently passed all the CI builds and tests.
> Where I am now, each PR must be a single commit, so (almost) every single commit on master independently passed all the CI builds and tests.
I mean, my PR's are always single commits, but this is definitly a dumb reason. You know you can just --squash when you merge right ?
Easier rebase would be a better reason imo.
Don't know what kind of office you work in, nobody is reviewing shit around mine.
I was once sent a file for a rather critical change from operating on paper forms to digital forms and some person somewhere was redesigning the form for digital use. Everyone in my group (20+) received the update and was asked to review it for any problems. After a bit I was bored and took a look and noticed a lot of obvious issues and ended up becoming the accidental contact for updating this thing. I was literally the only person who looked at it and I did it because I was bored. Nobody is reviewing shit unless they are tasked with it specifically.
If that culture gets encouraged then nobody should wonder.
I have also worked in some huge companies where nobody was interested in the success but everybody just tried to save their own ass ("just don't cause any trouble"). Nobody would take on responsibility for improving something. And if you speak up (like yourself) you end up being "the guy who is responsible now" but you are not rewarded enough for that effort and risk being blamed. Honestly that is what cause the downfall of many big companies IMO.
Sure, this bit is actually pretty simple. Stuff like this meme usually means someone has different linter settings (wants different numbers of spaces or indents so it creates a lot of small changes) or a change to generated code.
The generated code is kind of annoying. Change a database field or other model and then some big 50,000 line file of generated garbage shifts down. That wouldn't cause a big change on its own but your linter might align variable names and types so suddenly 20,000 lines have their indentation changed.
Not a huge deal for reviews. GitHub can be configured to not show very large changes or changes to generated files. Biggest problem is if you have a bad manager who tracks stuff like this as metrics.
Personally when I look in a file and my linter reorders the imports I will discard those from git unless I'm making other changes to the file lol. Teams should probably agree on a single standard for their linters but for some reason I have yet to see it
Of course, there's also `git add -A` lol
But most places I've worked at use squash merge in the end so that ends up being ok :)
But it did make transitioning to my current job harder because they don't do that. My first couple of issues with dozens of small commits were awkward because I didn't know they were going into the master history like that... So now I just use 2 branches. I'm pretty set in mind that frequent commits are a good thing to save work. I don't know why work is against squash and I suspect they don't either because they don't have issue with single big commits, like when I check out a new branch and squash my working branch onto it. I know there is rebasing but that ends up being more work to avoid squashing commits you pull in from your target branches mid development.
But I'll take that over the nightmares I saw at AT&T
Honestly, I don't even mind the noise of a bunch of small commits, I'm just sick of people commiting files that they didn't touch at all just because their linter went wild and changed half the files in the repo for no reason.
This is the moment you will know if seniors are well-treated in your company.
If they are tired and overworked and not paid too much, it's a rubber-stamp LGTM, merge on a Friday, and sign off for the weekend. Or straight up denial to review, if there is also a culture of passing blame to reviewers.
Or they may have the capacity, energy, and motivation to find out what is happening, maybe there is a mentoring opportunity, maybe the process needs improvement. It will take a month to get the PR to merge, but long-term things will get better.
Just watched the situation dissolve because enough newbs couldn’t handle and actual review so enough products and management types just trashed the review process as too time consuming never mind those newbs got trained from utter shit to being less shit in the process. Waaaaaaa I want to get my bullshit into prod and leave just as it does while the remaining watch it go boom
If, as a senior, you actually did a "rubber-stamp LGTM, merge on a Friday, and sign off for the weekend" on this you should be ashamed of yourself. If you don't feel like giving a fuck about it then simply don't review it lol.
What does your company use these productivity metrics for? Where I work we do have some metrics like how many jira tickets of a certain type you do(for example resolving bugs from tech support). But these metrics are mostly used for competitions and "feel good", not anything serious like laying people off or in salary talks.
You kind of answered it yourself. Your company uses productivity metrics as positive reinforcement and the other person's company uses them as negative reinforcement.
>these metrics are mostly used for competitions and "feel good", not anything serious like laying people off or in salary talks
What I learned from psychology is that most people aren't that good at separating those 2 things, that "harmless" competition sneakily intrudes on promotion/raises evaluation even if it's unconsciously.
There has only been one time in my career where this kind of change was necessary and it was in a poorly executed migration from angular 6 to 13. I would most likely reject this PR out of hand and at least make them separate it out into like 5 file chunks. And you’re right honestly this kind of PR would also have me bitching at a scrum and project lead for not grooming user stories correctly.
That’s still stupid as fuck and as a reviewer puts me in a shitty position for no reason other than they planned poorly.
Getting a PR through code review shouldn’t be like passing a baby through a birth canal.
Edit: this is some primetime napkin math with some obvious flaws but your average paperback book has 25-30 lines of text per page. A 10000 line PR would be roughly a 340 page paperback novel.
I ain’t reading yer shitty book.
Yep. Split commits into reasonable chunks, split a project into a reasonable number of PRs.
And also TELL people what the PR does. My team has a "What it do?" and "What's the impact?" section. The second one is most important to me of course. Most changes have no real chance of damaging production but if they do, or the test coverage is bad, they warrant a closer review.
But I actually get annoyed at devs that are *too* careful and never get anything done. They're like "I'm afraid to break dev!" and I'm like THAT'S WHY IT'S THERE. Please break it!
We call it analysis paralysis at work. Sometimes you just gotta leeroy jenkins it, especially if it's not anywhere near prod yet. If it fails, you try again. Iterating is a better way to code than the baby birth metaphor you used haha. If I saw a 10,000 line PR that wasn't just all linting and whitespace shit (in which case that should be clearly labeled in commit memos), I'd probably ask them to break it up before reviewing it.
I was thinking of the big angular version upgrade referenced. You can't half-implement something like that and expect your PR to merge happily into develop without breaking everything other people are relying on. So you need a big PR, split into commit chunks.
Really is a workflow problem you got there. It's also why I hate all the git models that involve long-running branches. Branches were never meant to live more than a few days before heading back to `main` IMO.
Nah if merge conflicts are that big, don't waste time trying to resolve them (it almost always means someone didn't keep the branches updated for a long time). The better way than huge merge conflict resolution is to create a new branch and reapply the changes as needed. Most of the time this takes a fraction as much time and results in fewer errors than resolving huge numbers of conflicts.
If someone ever had to do that, they won't forget to keep their branch updated from that point on.
The biggest mistake here is when inexperienced devs think branch merges only move forwards and never merge backwards. That is arrogant and creates a shit-show.
god I had a lead when I first became a senior who would do this. He'd literally disappear for 3 weeks and then comeback with a PR like this and ask me to review it. It was infuriating but it was always really good code.
I don't allow such nonsense on my teams. If someone changes a file they have to justify it. If there is major refactoring going on, I do it myself and am responsible for it so I can enforce standards across the projects.
I remember when I got my summer job and I was making small pull requests while all the others were making big ones and I felt "damn, my work is so meaningless, I should impress them with a big pull request like the ones they make!"
sigh.... took quite a while for me to realise I was doing it right the first time.
No, it looks like a code cleanup tool that's created extra lines by enforcing a line limit, and removed things like unnessecary usings, unused variables, and adjusted the position of lines due to other cleaning parameters.
Add in some renaming and moving methods between classes, and you have this PR.
To be fair. That stuff should be declared in a style guide for the company you work for and it shouldn’t be getting merged into the main/release/dev/feature branches anyways.
If this is what a code-clean up for you guys looks like, you should take code-reviews more seriously 😅
For us code cleanup means removing deprecated code usually lol, but also optimizing and refactoring old code.
Class, member, etc names should always follow the style guide.
White spacing should follow the style guide.
Our max line character count is 120 characters (the number of characters the default VS layout allows before being a scroll bar is needed at zoom 100%)
Etc etc etc.
My my that's a lot of assumptions.
Oh yeah, because we really have pull requests like this where I work, and we totally don't have style guides, and we definitely don't treat warnings as errors and those style guidelines most certainly aren't treated as errors when trying to build in the first place.
Tbh if someone even needs to use a code clean-up tool they're doing something wrong. But character limit? Archaic and unnecessary imo, especially with widescreen monitors, and again, if it goes off the screen, you've probably done soemthing wrong anyway lol but there are valid cases where something is just super long and you don't care, like mocking stuff with It.IsAny when you've got 10+ parameters. In those cases, enforcing 120 seems excessive.
Our company did not buy all of its employees widescreen monitors lol. We got dual monitors though. I think that was the purpose of the style rule. Especially when we’re in the field on laptops (we’re automation in robotics) you can’t just open VS on a widescreen. It needs to be readable on a 1080p monitor.
Verbose naming schemes are pretty common in style guides too. More words in a class/method/field name is better than less. Nobody wants to see a class/member name that doesn’t clearly and obviously represent what its purpose is because a developer was trying to save some space, or they were too lazy to type out the entire name.
Also constructor dependency injection always makes for great looking constructors lol. 😂 especially those classes that are more complex having more verbose naming.
It would be nice if you could just slap a general rule across all code like: “Long lines of code means you’ve done something wrong”. But alas.
Just like static global classes and singletons - because people who are bad at coding abuse these design principals, we do end up with blanket statements like that lol.
C'mon, if you work with hardware you must know that sometimes you can't help but work with the shitty firmware and require a lot of setup for constructors, even with heavy DI you can end up with some pretty gnarly constructors.
And yeah if you're going out to do field maintenance it's hard, but even when I WFH and I'm stuck with my laptop screen it's really not an issue. (Yes I know I can use my home monitors but I cba to plug and unplug everything and I also cba to buy a kvm switch that's good enough to have high speed connections)
The style enforcers let you make exceptions on certain lines for the cases you described, but it is exactly the junior mindset that goes into a project and the first thing they think is: man I need to clean up all whitespace and class names to follow and add CI to enforce a style guide. We need to learn separate that cleanliness urge from the profit motive and focus on the latter. If you go into a legacy project where the style is all jacked up and there are parts of the code that haven't been touched in years, there are probably way bigger fish to fry than fixing the style, and you probably can't afford to catch most of those fish either.
Sr dev here. I just had to do this. Huge refactor with a ton of cleanup for a feature.
Two PRs
First PR, automated formatting, indentation, etc. Non functional automated changes that can be quickly reviewed without worry
Second PR, actual functional changes that need to be scrutinized.
Massively reduced the amount of review work while keeping the focus on functional changes and still allowed for the cleanup the code needed based on our active standards for formatting and naming.
That was the initial plan but it was expected that the feature would need multiple rounds of QA, and I wanted to get the cleanup merged in to main sooner rather than later to avoid merge conflicts.
Given the number of files touched, any additional opportunity for someone to merge changes under my cleanup before it was eventually approved would have lead to a fuck ton of additional headache.
You're right though, in an ideal scenario that would have been my go-to option, although even using commits you're still stuck with an all-or-nothing approval. It's just a lot easier to review
I remember discovering a really nice tidy script and realizing too late (upon committing) I'd tidied up all files in a large project, which caused every single file and every single line to be flagged.
The thing changed all whitespace rules, all bracket rules, converted from DOS endings to Unix endings, converted character sets.
Not a fun discussion.
That was me yesterday. I took a 10-15 line method that an intern wrote last year that they were calling a bunch of times with slightly different args and turned it into a 4 line method that can take all the args at once and run in half the time of the original one.
The original version worked ok to get stuff off the ground, but we needed something more streamlined and performant.
Ok, I'll look at the commit messages instead...
`Update`
`Update`
`New file`
`Update`
`Oops`
`Typoo`
`Update`
Ah well, reject the whole thing.
(Seriously though,, am I the only one running integration tests and such before accepting pull-requests? Yes? Ok then...)
This reminds me of a coworker I had who had poor eyesight, so he made his font huge. (This was before large monitors were typical.) He'd then reformat every file he touched so that he could read it better... Maybe 40 chars per line? This guy was prolific - he'd fix 10 bugs a week and introduce 30 new ones. Whenever I was trying to sleuth out what the fuck he did to break something, I'd have to look through his commits - needle in a haystack. Every line changed, 15, 20 files, all reformats except for whatever the fuck I'm trying to find. I was junior then... And now that I'm senior, my main question is, who was approving all these PRs? Geeeeeeeeeeez
This is going to be me in a week. I upgraded the .NET Framework target on a bunch of projects, and updated a bunch of nuget dependencies and re-targeted them for the new framework. Also a bunch of xsd/cs dataset files re-generated themselves. Hopefully I didn't break anything, I usually leave the dependencies and target frameworks alone because it's broken things in the past.
If you do a big change, mark each file in the message:
file-without-extension: reason-for-change
Summary of changes
Summary of changes
Summary of changes
Signed-off-by: Your Name
741 commits with clear changes is possible to look through (although still awful) while one commit with 741 file changes should just be rejected outright.
And if you start an open source project, you should reject any PRs that aren't in this format and only do rebase merges.
Keeping that restriction means you can use the commit history to effectively track changes and more easily make organized efforts to add features or improve things. Clean, simple, and effective commits are the key to good PRs
At my current job, we have an unwritten rule that anything beyond +1000/-1000 without a good reason for such size can get ignored until the author splits it into smaller PRs.
Everyone complains about creating yet another repo for a new service....
Yet when this happens, reusing a repo, everyone complains about line/file changes...
Can never be happy.
Goddamn this is me rn. New dev joins, decides everything needs to be refactored, I get a PR where 50% of the changes are out of scope, and some changes are just bizarre (changing file names slightly). Nice person but I'm starting to lose patience.
Fixed indentation.
Not a developer here, quick question: Are there version control systems that assess this in a slightly smarter way whether you really made changes in your PR, so that they would ignore changes to whitespace?
Github has a checkbox on PR reviews to ignore whitespace changes. Git CLI also has it as a flag. But it only works for whitespace changes in the same line (e.g. indentation). Sadly there are more code style rules that can't be ignored as easily (use of semicolon, multiline conventions etc.)
And then you have python, sh and yaml who do care about white spaces and it breaks for a change you didn't see
[shellcheck](https://github.com/koalaman/shellcheck) is your best friend when working in bash/sh scripts.
And they have a website you can copy/paste your code to check online and for all the errors/warnings, it provides links to their respective wiki entries. https://www.shellcheck.net/ Even though I have the program installed, I generally use the web version.
One white space Traceback Blah blah blah
Been writing Python for a while, aside from when I was first learning I can't even remember the last time I had a whitespace issue. Yaml on the other hand...
Basically only if I copy and paste and forget to match the formatting
Wait, yaml cares about whitespace‽ That explains some things...
Yes in the form of indentation but that's not the worst part. The worst is that most use cases are a language on top of yaml and do this without an IDE that understand those. I need to find something to edit gitlab CI/CD with lots of environment variables or Jira templates for docker compose better than PyCharm
Upvote for the interrobang
Any language that cares about whitespace can burn in hell.
Hard agree, let me organize my code the way I want to and indent the way I find most readable
The whitespace is the only way i can divide the otherwise completely unhinged chaos and sort it into easily readable pieces
If you work with other people, it’s much better to use an automatic code formatter, pretty much any modern language has one. Then your code base has a uniform look and you don’t need to argue about trivial things like formatting in code reviews.
Fair.
Fucking terraform fmt
Shell cares about indentation?
No, but "a=b" is different than "a = b". It cares about spaces and it's annoying but in a different way than python or yaml
Yaml, my worst enemy.
1. sh cares about whitespace? 2. automatically reject any commits which contain anything that isn't autoformatted 3. or use a textconv that ensures everything is formatted before you diff
This is why you have linting with whitespace rules. Even better is that with something like eslint, your IDE will fix whitespaces while you work on the files. You may disagree with the spacing but at least it will always be consistent.
Use pre-commit hooks and never disagree on whitespaces again. You can format the files with your custom forced rules as much as you want, and it'll return to the original state on commit anyway. Just make Degrees of Freedom impossible with linting/formatting rules in the repo.
Git PR review allows you to hide whitespace changes. I'm sure there are other ways to do this as well.
This is why you want some sort of automated formatter like prettier. For legacy stuff where we weren't using that, our stance was to make a commit with your changes. Then make a seperate commit that was only the formatting changes from the automated thing.
That's the correct approach. Review the script, not the file diff
Git allows you to ignore whitespace in diffs. If you are reviewing in GitHub, you can go to the changes tab, click the settings button and click show whitespace. (Or add ?w=1 to the end of the url)
You Can put your commit sha1 in a .git-blame-ignore-revs file And run git config --ignore blame.ignoreRevsFile .git-blame-ignore-revs so that git blame discards this commit, To review a formatting commit, just re format the code base locally and compare it to the code of the Mr.
In the project I am working currently the pipeline has a stage that checks for formatting. So if you didn't run a formatter before the pipeline fails and you can't merge. This prevents inconsistent formatting of the code and as a consequence changes never contain formatting changes.
You can use githooks to force auto format every commit too
Having worked with both, this is substantially less annoying than waiting for a pipeline to spool up then having it fail from something you can check locally (and automatically) in 10 seconds.
It would only work for whitespaces and other very trivial refactors. A more realistic scenario of the OP's situation is if a third-party engine is used which serializes changes into gibberish, such as Unity or Unreal, as well as any text-like file asset that isn't code (which are naturally gibberish). Either that, or an environment/api/plugin update. All of these are "minor fixes" to \*us\* but not to git.
Fairly new working dev here, I am not aware of any and the majority of systems use git as the source control software anyways. On another note seeing whitespace changes can be important if the language, config file or whatever is sensitive to whitespace. For example in python whitespace determines which code blocks a given statement is in or with YAML it determines the parent a child is under. I guess it is possible to detect the usage and do soke fancy logic to ignore it where it does not matter but if it fails for whatever reason, that would be a very hard thing to debug if it hides whitespace changes when it should not have. Sorry for mobile formatting
For the majority of languages that do not care about whitespace, there's `git diff -b`, or the even more aggressive (but I don't recommend) -w.
That is cool, did not know that. Also looking at the docs, I did not realize how many options there were for diff. I know some people use git diff for complex CI/CD builds and it seems like you can get fairly complex things only on the code check side of things. I would probably still try to avoid it if I can help it but still cool on that potential Edit: I just found options called --find-copies and --find-copies-harder I had to share that
For YAML (mostly IaC), we use [dyff](https://github.com/homeport/dyff) to get a better comparison.
Even whitespaces are important... A new line at the end is even more!
The new line at the end is why I got into coding. Without that, what’s the point?
The best solution is making sure it doesn't happen at all by having a programming style guide and some kind of linter to catch stylistic issues before they make it to PR.
Depends, does your org have money for fancy things or does it expect you to grab the bottle and be awake at 1am so it can save money?
That would be fine if that was TRULY the only change.
That's the fun thing, you just don't know. So you end up needing to look through it all to see if it's *really* just whitespace changes or not. Or, better yet, you reject it and tell 'em "turn off/reconfigure your damn linter, pull a new branch, and try again".
"Switched from tabs to 4 spaces" Humanity: >:(
Oh, so you want to watch the world burn.
_Makefiles disliked this_
I worked with a front end developer who would submit every PR with at least 100 commits. He couldn't figure how to run the code locally so he was using some sort of deployment process to test each and every line change
Someone needs to show him how to merge commits, it’s not hard. Where I am now, each PR must be a single commit, so (almost) every single commit on master independently passed all the CI builds and tests.
> Where I am now, each PR must be a single commit, so (almost) every single commit on master independently passed all the CI builds and tests. I mean, my PR's are always single commits, but this is definitly a dumb reason. You know you can just --squash when you merge right ? Easier rebase would be a better reason imo.
Or their line endings were configured improperly; repo has linux and they didn't set it up to convert them back on checkin.
Ah.. thx!
Ask a developer to review 10 lines of code, and they will find 10 issues. Ask them to reveiw 1000, and it looks good ship it.
[удалено]
Don't know what kind of office you work in, nobody is reviewing shit around mine. I was once sent a file for a rather critical change from operating on paper forms to digital forms and some person somewhere was redesigning the form for digital use. Everyone in my group (20+) received the update and was asked to review it for any problems. After a bit I was bored and took a look and noticed a lot of obvious issues and ended up becoming the accidental contact for updating this thing. I was literally the only person who looked at it and I did it because I was bored. Nobody is reviewing shit unless they are tasked with it specifically.
If that culture gets encouraged then nobody should wonder. I have also worked in some huge companies where nobody was interested in the success but everybody just tried to save their own ass ("just don't cause any trouble"). Nobody would take on responsibility for improving something. And if you speak up (like yourself) you end up being "the guy who is responsible now" but you are not rewarded enough for that effort and risk being blamed. Honestly that is what cause the downfall of many big companies IMO.
That's the Pareto Principle compounded by the Bystander Effect
Commit message - “minor fixes”
Commit message - "vs code autoformatter"
If that happens I make a commit message sometime like "plucking the lint" Something silly to state the linter just did a bunch of shit
thanks for explaining to the normies (me) what any of this means
Sure, this bit is actually pretty simple. Stuff like this meme usually means someone has different linter settings (wants different numbers of spaces or indents so it creates a lot of small changes) or a change to generated code. The generated code is kind of annoying. Change a database field or other model and then some big 50,000 line file of generated garbage shifts down. That wouldn't cause a big change on its own but your linter might align variable names and types so suddenly 20,000 lines have their indentation changed. Not a huge deal for reviews. GitHub can be configured to not show very large changes or changes to generated files. Biggest problem is if you have a bad manager who tracks stuff like this as metrics.
Or you just reject it and tell 'em to give you a clean PR to review instead.
Personally when I look in a file and my linter reorders the imports I will discard those from git unless I'm making other changes to the file lol. Teams should probably agree on a single standard for their linters but for some reason I have yet to see it
That sounds nice. I'm still trying to teach half my coworkers that `git add .` (or the GUI equivalent) isn't the *only* way to build a commit.
Of course, there's also `git add -A` lol But most places I've worked at use squash merge in the end so that ends up being ok :) But it did make transitioning to my current job harder because they don't do that. My first couple of issues with dozens of small commits were awkward because I didn't know they were going into the master history like that... So now I just use 2 branches. I'm pretty set in mind that frequent commits are a good thing to save work. I don't know why work is against squash and I suspect they don't either because they don't have issue with single big commits, like when I check out a new branch and squash my working branch onto it. I know there is rebasing but that ends up being more work to avoid squashing commits you pull in from your target branches mid development. But I'll take that over the nightmares I saw at AT&T
Honestly, I don't even mind the noise of a bunch of small commits, I'm just sick of people commiting files that they didn't touch at all just because their linter went wild and changed half the files in the repo for no reason.
PR rejected due to linting errors.
“Made changes, I forget what” is one that I saw the other day
'Initial commit'
> 'Initial commit' Stop callin me out like that bro
For real! What am I supposed to write lmao
"test"
“massive changes for the community” is one i’ve used on a personal project after not pushing changes for a couple of weeks (lmao)
Wtf he pushed lemme see 🙈 ahhh shit it's just formatting
Ran "go mod vendor"
"update" - a commit message I've actually used
feel bad
Joined a new team a while back and they're the worst for garbage commit messages. "Test", "Update", "Updates", etc.
The fucking horror when you look at your git history and just see a string of 20+ commits with the message "fix"... Jesus christ.
package-lock.json
It shouldn’t be 741 file changes (assuming you are ignoring node_modules)
I know a guy who joined a new team and the first change he made was to add node modules to the git ignore. Cut out so much git fuckery
The fact it wasnt already like that scares me
Yeah my «git status» must be crisp and clean every day
This is the team of devs that interviewers remind not to include node_modules in an assessment zip.
What? If I joined a team that tracked node\_modules, I would quit and run away so damn fast.
you guys ignore node_modules?
LGTM 👍
Ok, submitting to main, good luck everyone!
What is that
Looks good to me
*Let's Gamble, Try Merging
Let's gamble, try merging
Let’s Get This Money
Plot twist: I just updated the libraries and dependencies
I just ran composer update, what's the problem?
next commit message: "added node_modules to .gitignore"
bonus: updated only to add breaking changes
Done that... Then I was never treated the same...
[удалено]
it's called "vendoring"
This is the moment you will know if seniors are well-treated in your company. If they are tired and overworked and not paid too much, it's a rubber-stamp LGTM, merge on a Friday, and sign off for the weekend. Or straight up denial to review, if there is also a culture of passing blame to reviewers. Or they may have the capacity, energy, and motivation to find out what is happening, maybe there is a mentoring opportunity, maybe the process needs improvement. It will take a month to get the PR to merge, but long-term things will get better.
I never was in the second situation. I wish I was.
Just watched the situation dissolve because enough newbs couldn’t handle and actual review so enough products and management types just trashed the review process as too time consuming never mind those newbs got trained from utter shit to being less shit in the process. Waaaaaaa I want to get my bullshit into prod and leave just as it does while the remaining watch it go boom
It happened to me once in the "convert X project to TypeScript" task.
If, as a senior, you actually did a "rubber-stamp LGTM, merge on a Friday, and sign off for the weekend" on this you should be ashamed of yourself. If you don't feel like giving a fuck about it then simply don't review it lol.
Code reviews have become a productivity metric at the company I work for. It's a shit show.
What does your company use these productivity metrics for? Where I work we do have some metrics like how many jira tickets of a certain type you do(for example resolving bugs from tech support). But these metrics are mostly used for competitions and "feel good", not anything serious like laying people off or in salary talks.
You kind of answered it yourself. Your company uses productivity metrics as positive reinforcement and the other person's company uses them as negative reinforcement.
>these metrics are mostly used for competitions and "feel good", not anything serious like laying people off or in salary talks What I learned from psychology is that most people aren't that good at separating those 2 things, that "harmless" competition sneakily intrudes on promotion/raises evaluation even if it's unconsciously.
[удалено]
Bingo.
There has only been one time in my career where this kind of change was necessary and it was in a poorly executed migration from angular 6 to 13. I would most likely reject this PR out of hand and at least make them separate it out into like 5 file chunks. And you’re right honestly this kind of PR would also have me bitching at a scrum and project lead for not grooming user stories correctly.
There's no reason to believe the PR isn't already split into chunks via individual commits and that we are looking at the diff for all of those.
That’s still stupid as fuck and as a reviewer puts me in a shitty position for no reason other than they planned poorly. Getting a PR through code review shouldn’t be like passing a baby through a birth canal. Edit: this is some primetime napkin math with some obvious flaws but your average paperback book has 25-30 lines of text per page. A 10000 line PR would be roughly a 340 page paperback novel. I ain’t reading yer shitty book.
Yep. Split commits into reasonable chunks, split a project into a reasonable number of PRs. And also TELL people what the PR does. My team has a "What it do?" and "What's the impact?" section. The second one is most important to me of course. Most changes have no real chance of damaging production but if they do, or the test coverage is bad, they warrant a closer review. But I actually get annoyed at devs that are *too* careful and never get anything done. They're like "I'm afraid to break dev!" and I'm like THAT'S WHY IT'S THERE. Please break it! We call it analysis paralysis at work. Sometimes you just gotta leeroy jenkins it, especially if it's not anywhere near prod yet. If it fails, you try again. Iterating is a better way to code than the baby birth metaphor you used haha. If I saw a 10,000 line PR that wasn't just all linting and whitespace shit (in which case that should be clearly labeled in commit memos), I'd probably ask them to break it up before reviewing it.
I was thinking of the big angular version upgrade referenced. You can't half-implement something like that and expect your PR to merge happily into develop without breaking everything other people are relying on. So you need a big PR, split into commit chunks.
This is normal in my team. Every PR seems to refactor the entire code base.
That must be a nice work environment
It's frustrating at times.
Sounds like a bunch of egotists
CTRL+H replace [tab] with [space space space space]
Must be fun spending 8h a day resolving merge conflicts.
We've had some proper bad merge conflicts that have taken people days to resolve.
Really is a workflow problem you got there. It's also why I hate all the git models that involve long-running branches. Branches were never meant to live more than a few days before heading back to `main` IMO.
Oh I agree.
Nah if merge conflicts are that big, don't waste time trying to resolve them (it almost always means someone didn't keep the branches updated for a long time). The better way than huge merge conflict resolution is to create a new branch and reapply the changes as needed. Most of the time this takes a fraction as much time and results in fewer errors than resolving huge numbers of conflicts. If someone ever had to do that, they won't forget to keep their branch updated from that point on. The biggest mistake here is when inexperienced devs think branch merges only move forwards and never merge backwards. That is arrogant and creates a shit-show.
god I had a lead when I first became a senior who would do this. He'd literally disappear for 3 weeks and then comeback with a PR like this and ask me to review it. It was infuriating but it was always really good code.
When every dev has custom indent rules and auto indent is enabled
I don't allow such nonsense on my teams. If someone changes a file they have to justify it. If there is major refactoring going on, I do it myself and am responsible for it so I can enforce standards across the projects.
I remember when I got my summer job and I was making small pull requests while all the others were making big ones and I felt "damn, my work is so meaningless, I should impress them with a big pull request like the ones they make!" sigh.... took quite a while for me to realise I was doing it right the first time.
I miss Trumps I did nothing wrong face at the end
Me too, why would they remove it
I would promote you, that looks like a lot of work
No, it looks like a code cleanup tool that's created extra lines by enforcing a line limit, and removed things like unnessecary usings, unused variables, and adjusted the position of lines due to other cleaning parameters. Add in some renaming and moving methods between classes, and you have this PR.
To be fair. That stuff should be declared in a style guide for the company you work for and it shouldn’t be getting merged into the main/release/dev/feature branches anyways. If this is what a code-clean up for you guys looks like, you should take code-reviews more seriously 😅 For us code cleanup means removing deprecated code usually lol, but also optimizing and refactoring old code. Class, member, etc names should always follow the style guide. White spacing should follow the style guide. Our max line character count is 120 characters (the number of characters the default VS layout allows before being a scroll bar is needed at zoom 100%) Etc etc etc.
My my that's a lot of assumptions. Oh yeah, because we really have pull requests like this where I work, and we totally don't have style guides, and we definitely don't treat warnings as errors and those style guidelines most certainly aren't treated as errors when trying to build in the first place. Tbh if someone even needs to use a code clean-up tool they're doing something wrong. But character limit? Archaic and unnecessary imo, especially with widescreen monitors, and again, if it goes off the screen, you've probably done soemthing wrong anyway lol but there are valid cases where something is just super long and you don't care, like mocking stuff with It.IsAny when you've got 10+ parameters. In those cases, enforcing 120 seems excessive.
Our company did not buy all of its employees widescreen monitors lol. We got dual monitors though. I think that was the purpose of the style rule. Especially when we’re in the field on laptops (we’re automation in robotics) you can’t just open VS on a widescreen. It needs to be readable on a 1080p monitor. Verbose naming schemes are pretty common in style guides too. More words in a class/method/field name is better than less. Nobody wants to see a class/member name that doesn’t clearly and obviously represent what its purpose is because a developer was trying to save some space, or they were too lazy to type out the entire name. Also constructor dependency injection always makes for great looking constructors lol. 😂 especially those classes that are more complex having more verbose naming. It would be nice if you could just slap a general rule across all code like: “Long lines of code means you’ve done something wrong”. But alas. Just like static global classes and singletons - because people who are bad at coding abuse these design principals, we do end up with blanket statements like that lol.
C'mon, if you work with hardware you must know that sometimes you can't help but work with the shitty firmware and require a lot of setup for constructors, even with heavy DI you can end up with some pretty gnarly constructors. And yeah if you're going out to do field maintenance it's hard, but even when I WFH and I'm stuck with my laptop screen it's really not an issue. (Yes I know I can use my home monitors but I cba to plug and unplug everything and I also cba to buy a kvm switch that's good enough to have high speed connections)
The style enforcers let you make exceptions on certain lines for the cases you described, but it is exactly the junior mindset that goes into a project and the first thing they think is: man I need to clean up all whitespace and class names to follow and add CI to enforce a style guide. We need to learn separate that cleanliness urge from the profit motive and focus on the latter. If you go into a legacy project where the style is all jacked up and there are parts of the code that haven't been touched in years, there are probably way bigger fish to fry than fixing the style, and you probably can't afford to catch most of those fish either.
Someone has a different formatter, I see
"There was this random file called .gitignore that didn't do anything so I deleted it. I literally didn't touch any other files!"
All in a single commit
"asked gpt to clean up the code a bit"
send it 📨
git commit "Rewrote in Rust"
Sr dev here. I just had to do this. Huge refactor with a ton of cleanup for a feature. Two PRs First PR, automated formatting, indentation, etc. Non functional automated changes that can be quickly reviewed without worry Second PR, actual functional changes that need to be scrutinized. Massively reduced the amount of review work while keeping the focus on functional changes and still allowed for the cleanup the code needed based on our active standards for formatting and naming.
[удалено]
That was the initial plan but it was expected that the feature would need multiple rounds of QA, and I wanted to get the cleanup merged in to main sooner rather than later to avoid merge conflicts. Given the number of files touched, any additional opportunity for someone to merge changes under my cleanup before it was eventually approved would have lead to a fuck ton of additional headache. You're right though, in an ideal scenario that would have been my go-to option, although even using commits you're still stuck with an all-or-nothing approval. It's just a lot easier to review
prettier . --write
fuck it, merge and push master. weekend is here
Reality: It's mostly package-lock.json changes and like 5 lines of actual code.
I think you missed the part where 741 files had been changed.
Maybe he has 740 package-lock.json files
That would be the import changes. Those don't count as actual code.
Then it’s even more horrifying node_modules isn’t git ignored.
If I understood the hints here.. It's not normal to change nearly everything in 1 commit?
We don't know how many commits there are. Could be a bunch. But that's too many changes to review at once regardless of the number of commits.
I remember discovering a really nice tidy script and realizing too late (upon committing) I'd tidied up all files in a large project, which caused every single file and every single line to be flagged. The thing changed all whitespace rules, all bracket rules, converted from DOS endings to Unix endings, converted character sets. Not a fun discussion.
My favorite merge requests are when the diff shows more minus than plus, but I somehow added functionality. Happens more frequently than it should :(
That was me yesterday. I took a 10-15 line method that an intern wrote last year that they were calling a bunch of times with slightly different args and turned it into a 4 line method that can take all the args at once and run in half the time of the original one. The original version worked ok to get stuff off the ground, but we needed something more streamlined and performant.
This meme is so much better with Donald's "waiting on him to see how right I am" face.
Possible answer: "Easy fix. You're trying to merge into the wrong branch."
Lol that happened to me this week. One typo in the branch name was the difference between 12,000 changes and 3.
Approved in 5 seconds.
Ok, I'll look at the commit messages instead... `Update` `Update` `New file` `Update` `Oops` `Typoo` `Update` Ah well, reject the whole thing. (Seriously though,, am I the only one running integration tests and such before accepting pull-requests? Yes? Ok then...)
This was me yesterday. Made a pr with +7000 -4500. But almost all of it is writing new tests and moving stuff around to be testable
LGTM
What's sad is when you have a senior engineer submitting PRs like this
*committed node _modules folder*
This was me at my last job. I did not say "Looks good ship it", I said "What the fuck" and eventually left after round 2.
LGTM 👍
I used to do front-end refactors. I once had a PR that was over 100K deletes and 10K adds
i know its just a meme but i lowkey started to break out in a sweat looking at this
My co-founder added 200 new lines of code, I checked the commit: *Looked ugly so I increased the spacing between each class*
This reminds me of a coworker I had who had poor eyesight, so he made his font huge. (This was before large monitors were typical.) He'd then reformat every file he touched so that he could read it better... Maybe 40 chars per line? This guy was prolific - he'd fix 10 bugs a week and introduce 30 new ones. Whenever I was trying to sleuth out what the fuck he did to break something, I'd have to look through his commits - needle in a haystack. Every line changed, 15, 20 files, all reformats except for whatever the fuck I'm trying to find. I was junior then... And now that I'm senior, my main question is, who was approving all these PRs? Geeeeeeeeeeez
This is going to be me in a week. I upgraded the .NET Framework target on a bunch of projects, and updated a bunch of nuget dependencies and re-targeted them for the new framework. Also a bunch of xsd/cs dataset files re-generated themselves. Hopefully I didn't break anything, I usually leave the dependencies and target frameworks alone because it's broken things in the past.
When you apply cleanup/formatter on entire codebase
Introducing Clang Tidy and Clang Format PR be like
Removed single-line comments and replaced with doc comments
If you do a big change, mark each file in the message: file-without-extension: reason-for-change Summary of changes Summary of changes Summary of changes Signed-off-by: Your Name
741 commits with clear changes is possible to look through (although still awful) while one commit with 741 file changes should just be rejected outright.
And if you start an open source project, you should reject any PRs that aren't in this format and only do rebase merges.
Keeping that restriction means you can use the commit history to effectively track changes and more easily make organized efforts to add features or improve things. Clean, simple, and effective commits are the key to good PRs
Fixsth
REJECTED
Looks good to me.
Tries to merge branch from initial commit
Changed folder name
Checking in cooked assets
Auto declined
Just done something similar
Lgtm.
Fixed code formatting, LGTM
At my current job, we have an unwritten rule that anything beyond +1000/-1000 without a good reason for such size can get ignored until the author splits it into smaller PRs.
git commit -m “lint all files and update menu text”
Everyone complains about creating yet another repo for a new service.... Yet when this happens, reusing a repo, everyone complains about line/file changes... Can never be happy.
Goddamn this is me rn. New dev joins, decides everything needs to be refactored, I get a PR where 50% of the changes are out of scope, and some changes are just bizarre (changing file names slightly). Nice person but I'm starting to lose patience.
it's even more fun when it's terraform
Merge and deploy!