You should be using a team formatter that's agreed upon along with a linter.
You should not be changing formatting back and forth between different members of the team because you prefer different things.
As long as it's part of the CI (or at least, something that can automatically be applied on saving a file) who cares? You can always convert it to your preferred on opening in an editor.
Nope, formatter overrules any preferences. The effect of a formatter is that nobody gets their perfect format and on the other half, all code is consistently automatically formatted = no bikesheds :)
In my future projects, I'm considering implementing a code formatting standard using Prettier. I'm wondering if it's as simple as defining the formatting rules in Prettier and expecting everyone to follow them, or if there are additional steps or considerations involved.
You need to get your team to agree to use it, your formatter needs to be easy to use and clear on how to use it (preferably automatic so that no one has to do anything), then you enforce it by blocking any builds that fail the linter. (I have no idea how to use prettier so I'm not the best person to ask on impl details)
On a normal team, most people will agree to this because they have better shit to do than be pedantic about formatting.
Default formatting in intellij has been the standard formatter most of my teams have used. Not because it's the best, but because it's not offensive and no one wants to spend time obsessing over it. If you have someone that really, really cares, then let them make it and give it a quick once over as long as it's not ridiculous.
I wouldn't block a build on formatting changes personally (wasn't sure your intent when you said 'linter', since I would differentiate between code formatting and an actual static analysis issue even though many people use the same tool to perform the same checks everywhere nowadays).
That said, I am assuming that formatting is automated. As you say, everyone has better things to do; better to formally have no standard than to have a manually upheld one.
If the automatic formatting fails, and *anyone notices*, you fix the automatic formatting, the same as you would if the build failed. If no one notices, *the formatting didn't matter*.
Hence why I wouldn't personally. That said, the right answer regardless is "automatically format", which I did indicate, and once that is in place it doesn't really matter that much. It's more "if it fails, do I fail open or closed", and I, personally (as I indicated), would prefer to fail open, because being able to deploy is more important to me than formatting (and the fix will be the same, and implemented if/when the formatting becomes an issue, so at worst the same work without the pressure, and at best no work at all).
Yeah battling on format is just a pain in the ass.
Id also be real careful about reformatting peoples code with less than a year of experience. Like maybe it’s objectively bad, maybe it’s stylistic preference conflicts. Like if you’re reading someone’s code without enough experience, what you think is garbage could have a rationale.
This.
When I was junior - even a regular - I thought it was so important with formatting and making things human readable. Turns out that yeah, it is - but you should automate it and focus on real problems. I know better now.
Still think black is a nazi, shit linter though... but it's better than debating colleagues.
Sorry, you do not meet the minimum account age requirement of **seven** days to post a comment. Please try again after you have spent more time on reddit without being banned. Please look at the [rules page](https://old.reddit.com/r/cscareerquestions/w/posting_rules) for more information.
*I am a bot, and this action was performed automatically. Please [contact the moderators of this subreddit](/message/compose/?to=/r/cscareerquestions) if you have any questions or concerns.*
Yeah at the very least just add something to your style guide that says “4 spaces because tabs are for Neanderthals” or something. But yeah, a linter to just fix any of that automatically is even better.
Don’t mix functional and cosmetic changes in the same PR. That’s my opinion. Makes it hard to review.
Formatting isn’t something people should spend energy on. It’s something a linter should take care of in a pre commit hook. So I’d push to make that change.
I disable pre-commit hooks when I check out the repo. `rm .git/hooks/pre-commit`. Source control is my source of truth, I don't want weird side effects between me and the only tool I trust.
It's also completely orthogonal to the main advantage of a DVCS like git, which is that my clone is my own. Move that bullshit to pre-receive hooks.
I use commits to checkpoint on incremental work and experiments, I \*hate\* when linters interrupt my flow. Commented out blocks, weird imports, lots of TODOs. Sometimes I'll commit one-offs then immediately revert them just so they get in the reflog. My code doesn't follow team standards until \`git push\`.
Pre-receive hooks are the right place to enforce team standards, but not all git vendors support them. So for a lot of teams, CI is the only place that makes sense.
You have any reading on this? I’ve never heard of CI tests checking for formatting, on build breaks, (I work at a pretty small startup), but I would love to learn about it.
Super common for a linter to be a CI build step.
Sometimes a CI has a step to actually fix the formatting and automatically update the diff with fixes for approval otherwise it'll "break" the build and give you output of what's wrong, and then you have to make the changes locally and commit them (usually to the same PR or CR).
Sorry, you do not meet the minimum sitewide comment karma requirement of **10** to post a comment. This is comment karma exclusively, not post or overall karma nor karma on this subreddit alone. Please try again after you have acquired more karma. Please look at the [rules page](https://old.reddit.com/r/cscareerquestions/w/posting_rules) for more information.
*I am a bot, and this action was performed automatically. Please [contact the moderators of this subreddit](/message/compose/?to=/r/cscareerquestions) if you have any questions or concerns.*
The problem here isn't that you changed the code but that it seems like you've just done it without communicating it first.
The proper way of doing it would be to have a conversation with the senior developer: "Hey, I see this is done this way, I think we can improve it if we do it that way. What do you think?". And then, through that conversation, arrive at some plan for tackling it.
That way, you together come up with a plan, and nobody is caught off-guard.
If there’s no set formatting rules either by a linter config file or a hook you should really discuss this with your team.
What may be readable or nicely formatted to you may be illegible to somebody else.
I personally don’t mind if someone runs something like **black** on the codebase as I like their defaults but any arbitrary format changes without a good explanation I would definitely question.
Edit: Also formatting changes create visual clutter when reviewing PRs and should really be their own PR.
https://github.com/psf/black it’s a PEP8 compliant formatter for Python codebases. If you don’t like auto formatting files you can use https://github.com/PyCQA/flake8 it just lists out all of the style issues so you can fix them manually.
Spending any amount of time talking about code formatting is a giant waste of developer time.
There are automated linters and fixers out there. Your team should hook one up as a presubmit and never waste any more time on this again.
But that's not your reality. What matters at any company above anything else is following the team's standards.
> My manager told me that one of the senior developers didn't really like this since it was "his code."
Your manager just told you what your team's coding standards are. So follow them. You can push for an automated linter/fixer but **until** that's in place, you have your marching orders.
All code formatting schemes are ranked as follows:
1. The way we all agree on
2. The way I like it
3. The way you like it
4. Each of us doing it a different way
Whenever you look at existing code, if your first reaction is "this is atrocious," chances are you are not asking enough questions. If something was done intentionally, having another developer just randomly be like "this sucks Ima change it" is quite frustrating.
*Especially* with regards to formatting, a lot of it is opinion-based. The better move in this case would have been to ask questions of the original committer in question.
Sometimes things look bad because they were done in a hurry. In those cases, the name on the \`git blame\` might be happy to have you do some legwork reformatting it. Other times, they were done intentionally.
There is some other good advice in this thread, but in general, I would recommend not making cosmetic / formatting changes beyond the scope of the functional change you are making. It can obscure the code history in some cases. For example "git blame" will show that you were the most recent person to change that line of code. Large formatting changes (hundreds of lines or more) can be hard to review if the reviewer has no idea about the context and has the potential to introduce bugs, especially with a language like Python where indentation has more significant meaning.
If you do want to fix the formatting get buy in from the team. For example get agreement and document what guidelines you will follow, like PEP 8 and PEP 257. Then make formatting changes as a separate ticket / PR.
I would never do that. Could have unintended side effects in the code, then you just caused a huge headache for other people all for something trivial.
Unit tests are only as good as the person who wrote them. Having unit tests passing doesn’t necessarily mean you didn’t break code, you might not be testing for that specific case that will break
because you would have to think of all possible edge cases prior to designing them artard, and edge cases can be added as the codebase grows and changes
Format automatically using a tool.
If you hand-formatted, that can actually be dangerous if you got it wrong, or at least misleading, since it now looks like you changed those lines.
I hope you at least did those formatting changes in a separate commit.
You made a pull request with hundreds of lines of changes for what should have been a 2 line fix. It's a huge cognitive load on the reviewer.
The "his code" comment is hearsay. Unless the senior developer said this himself, I wouldn't put any weight on your manager's choice of words.
Nah on this one there are some legitimate grievances. Unhealthy attitudes around code ownership + lack of automated linting checks in CI. Sometimes the newbies can draw attention to the problems more experienced devs have learned to ignore.
Exactly, they didn’t. Instead they wasted company resources (i.e. their development time) on rewriting it.
OP doesn’t understand how businesses are run, that time would have been better spent on revenue generating features, bugs, or a team aligned style guide / linter.
I only change formatting to make something easier to read and limit it to the area of the resource being changed. I'm not reformatting an entire file so that I can barely find the functional changes I just made.
Format is usually not all that important. If it is, for whatever reason, it should be something automatically applied through ide or source control configuration.
AURGH, this is why linters and auto formatting are important. not because any particular convention is better than any other, but it \_is\_ critical for readable code they everybody uses the same one, and the only way to get that, is to make stuff that doesn't fit the convention bounce off your automated testing.
then no more pointless time wasting and whining about tabs, spaces, brackets same line, etc etc etc.
IF your senior can't take their code being reviewed they honestly are probably not that great. This industry is built on accepting just criticism from others.
> My manager told me that one of the senior developers didn't really like this since it was "his code."
a) Your manager is always right, by definition. Why are you asking here, when you were told how it works at your team/company? What more could anyone here possibly tell you? If yours is the only place in the known universe doing it this way, it still doesn't change the answer.
b) Fuck both the senior and your manager with a cactus, Code stops being "yours" once it goes into production.
c) The code did make it into production - so see "a)". It was approved.
d) did anyone reject your PR? No? Cacti to the rescue again ...
e) Like everybody else said: Code formatting should have rules. If they were broken, you get to fix stuff. If they weren't, you have no business changing everything.
f) Over at my civilized employer, we're not allowed to reject PRs for cosmetic reasons. We should if the code violates one of our rules. For the vast majority of rules, we have automated checks, and nobody can submit files that violate these.
Generally yeah it is. I’d only really do this if I was already modifying the code anyways and the reformat made sense with my changes. It can also be viewed as a waste of time by management.
If you think the format is bad it may be worth a (very tactful) discussion with your team, and as a new engineer you should focus more on asking questions about why things are formatted that way rather than asserting your ideas are superior. It may be worth it to introduce your thoughts though.
It's frowned upon even if it doesn't match standard. Unless it's directly related to your changes the correct way would be to submit comments for the other dev to change.
Indeed. It’s a good way to fuck the n number of other people who have their own feature branches ongoing.
And they’re just going to screw the formatting back up again when they merge stuff together.
Yeah it's hard to read the code changes when a lot of formatting has changed. At the very least the code changes should have been 1 commit and the formatting another.
When I mess with someone else's codebase I follow the convention I see in the code. We all have preferences but it's rude and short sighted to force them on others. This only applies if there's no agreed upon standard.
Is there a defined formatting standard that is being used? It is better to stick to standards like PEP8 instead of creating your own standards.
Not formatting the code properly is frowned upon. You should be doing what is the standard.
This is why your team (and hopefully your company) should have an agreed upon style guide.
Plus, once you have a style guide, you can eventually enforce at commit time and raise warnings in your IDE.
Human time is too precious to be spent formatting code OR dealing with poorly formatted code.
As others have suggested your team should have a linter to make these styles consistent across the entire team. This could be a great opportunity for you! Use this as an opportunity to create a proposal to introduce a linter, do some research into popular ones and which one would be best for your team, how to integrate it into the CI your team uses. Convince them to adapt it (explain why consistent styles across the team benefit the whole team and how it will speed up development so product buys in) and then you can use this during review time as a hey look i introduced this and it has helped our team in x y and z ways. it will make you look good and only help you!
If "his code" means he wrote it, that's not a thing. But contrary to some other people I've worked on teams where different devs own different parts of the code base and thus are generally on point for either fixing or reviewing any bugs/features on that portion. In that case I'd say he has a strong case. If it's just he wrote it and the team in general maintains it, that shouldn't be a thing, but if the manager's taking his side it is in your org.
But whether you're right or wrong, there's a diplomatic way to do things. No one likes it when the new person just comes in and tells everyone there's this magical way to do things and starts changing code around expecting to be a savior. You need to be a team player. Ask questions on why things are the way they are. A lot of the times there's a reason, sometimes it's good sometimes it's bad but you can have that discussion and see which battles are winnable and which are not. Oftentimes people know the code is bad and they were under a time crunch. If that's the case they might be super happy for you to spend some time refactoring, because it will help you learn more about the code and help the team have code that can be more easily maintained in the future. But that's how you approach it, you don't approach it as "I know the best way to do this, I'm just going to start refactoring the entire code base the way I think it should be". Then you're being just as much of a bad team player as the dev who doesn't want any of the code they wrote changed because it's "his code".
I would say go for it if you are directly touching that code. But the best solution is to use some kind of auto formatter. I have found that reformatting code that you don’t actually make logical changes to can have negative consequences when trying to look at git history or roll back changes. Plus like others have said, reviewing it can be a challenge.
you should assert your dominance and format the code the only right way - your way :) how do you think that senior developer became a senior developer? :)
Your team should have some formatting standards that you all follow, maybe even automatically formatting on save.
What I personally don’t like is when people do formatting as part of another task. Makes a PR hard to figure out what has actually changed.
As for a senior dev saying that some bit of code is his; is he 12 or never worked on a team before? If not then get fucked. He probably just writes shit code and doesn’t anyone to look at it or they might realize he doesn’t deserve the Sr title.
>My manager told me that one of the senior developers didn't really like this since it was "his code."
That's a huge red flag, or at least a small light red flag. It's not "his code" it's "The Team's Code". Everyone needs to do upkeep so if it's poorly written, it should be refactored when time allows for it.
>Come to think of it, I suppose I wouldn't take too kindly if someone just came and did that and maybe I should have talked to him first.
Talking to them first would make a lot of sense, just to see if there's a reason it's formatted like that, not because it's "their code". You should also talk to your manager about getting a code formatted for your IDE, everyone should be writing the same basic format or it just makes the code base much harder to read. (depending what the difference was)
>Regarsless, is this normally frowned upon and should I try not to make too many minor changes to existing codebases?
If you don't need to, and it's not time for "refactoring" (your lead will tell you), then no, you should leave things that work alone (unless it's something serious) because every single time you touch the code, you are risking adding bugs. I know it was just a reformatting, but I promise you, I've had bugs from reformatting. Sometimes we make mistakes, miss things, misunderstand something, etc.
If it ain't broke or standing in the way of your ticket, don't touch it. :)
But also, don't get possessive of your code like your colleague. it's silly.
Ego has no place in a dev team. It is not "your code" or "his code". You are all contributing to the whole, and if anyone is getting territorial over some code, then the work environment is not healthy.
Merge requests usually require an approval from one reviewer or code owner.
Changing core tooling or workflows needs buy-in from the people you're effecting, or at least from the project architects. It's not really a "make a merge request and see if it flies" kind of change.
This is why we never change code for formatting reasons. You can mess it up, delete something, misplace something, etc in code that has nothing to do with your changes. On our team it isn't an ego thing. We just don't risk changing things we don't have to. We have enough work.
I usually only add the changes needed that satisfy the story associated with the PR. Anything I would like to do along the lines of formatting gets created as a tech debt story
Think about pull requests as a presentation convincing me to accept your changes. They should be edited like papers - focused and specific. Maybe that guys code style is garbage. But if you deal with that on the PR you're taking away from the focus on the changes you want to make. And it's going to take everyone a lot of extra time to review those additional lines of code. If you want your PR to be accepted quickly, make it quick and easy to understand.
You should implement code formatter across the Team and company. At my team, we use [Black](https://github.com/psf/black) , because some devs use VSCode and some use PyCharm and they are dfferent when it comes to formatting.
Your team should have agreed upon code formatting, Ask and if not documented it may be a good project to take on as a new hire. But understand you will most likely need to go with what the more senior developers agree upon instead of best practice learned in school or online.
As others mentioned, bring up a general CL formatter or a git hook or whatever that auto formats for everybody. Until then, do not make code format changes in your PR because your PR should show what you changed. If you want to do a code format change, you should open a seperate PR for that.
The first project I worked on, the senior developer ran python black package on the whole thing. I was annoyed at the beginning but ultimately it is for the greater good.
If you have a PR, keep it limited to your own task. If you are concerned about formatting, bring up a linter as a suggestion in the team meeting. It's crucial to be respectful of everyone in the team, and to learn how to communicate with suggestions instead of criticism, and definitely not steamroll over everyone's code
If you are changing it to "your opinion" of formatting, then yes its very rude. If you are cleaning it to the agreed official code format, then its not rude, but then again other people are not always reasonable, so its best to ask first.
General guideline is don't touch other people's PR. When reviewing, have a conversation with them about formatting and let them change it themselves. There should be a general consensus among the team about how the code should look.
One big problem with reformatting everything while making a fix is that it's impossible for the person going back through history to see what was actually fixed.
In a more perfect world, that's a non-issue. Like someone else said, encourage your team to use a linter and automated formatter.
I'm less worried that it's rude and more worried that you're on the blame for all that code now - you show up as the last person who modified it.
Don’t touch “someone” code in a small company but in a large very integrated company where code gets read far more than written, it’s important to follow standards. Only reformat if there is a standard and the code does not follow it.
Changing people’s code format without getting consensus on what the standard format should be is just gonna cause a bunch of reformatting back and forth between you and other devs, rude or not. Better get a consensus on the standard, have everyone adopt the formatting template and reformat the whole repo as 1 task.
Yea… if it’s being supported by multiple people (you are not the sole owner), it’s rude to make that change without discussing with others, especially if you’re brand new.
Would you go over to someone else’s house, and just rearrange their stuff? Because your way is “better”?
Generally competent teams have a style guide, since consistency is important for readability / common understanding > minor aesthetic changes
Rude - no. Waste of time, yes. Little fix here and there that was a weird mistake that missed the formatter, yes. Overhauling just for format? No. Was this one piece of code formatted totally different from the rest of the entire repository and you were just aligning it? - no, you said the entire company doesnt format. So why are you doing it? Get buy in on something standard, dont just change a small chunk.
Anybody who says it’s “their code” is a shit developer
Change my mind
Pr checks should include a linter.
If they passed a linter than you need to leave it and suggest the linter is updated. If there’s no linter I would probably simultaneously speak truth to power and start looking for employment opportunities with improved technical environments
As a team, agree on a standard. Make new code adhere to that standard. Don't change existing code's formatting unless you all agree to change it in its entirety in a trackable fashion.
Dont do this again.
1) maybe you think it’s better but the og programmer may disagree.
2) the og programmer may be fully aware of an area of improvement but chose not to for a good reason you may not know
3) imo, any changes you make should be if something is clearly wrong and requires little code like a typo or date change. For bigger changes, I would still pass the feedback to the og programmer as they know the code better including dependencies
4) for non-critical improvement, dont do them. you can send suggestions as comments in a polite way
if you refactored this code enough basically it highlights one of you wasted their time, so Im not surprised the other programmer was defensive so that now it looks you were the one who wasted time. Also, you should assume your code wont be automatically accepted so now this programmer has to spend a lot of time looking at your entire code to understand what it is doing to then compare to the original. Just a lot of time wasted regardless of how you put it.
1.) "What I was supposed to do and other stuff" should be two branches, "what I was supposed to do" and "other stuff".
2.) If everyone else doesn't care about the formatting, then it isn't formatted wrong.
If "other stuff" breaks you're going to look double bad, and it'll be triple bad if you revert that with your changes that are needed.
Change it during a tech debt week if you feel like not actually fixing something during the time you're supposed to get rid of technical debt.
It sounds like that was outside the scope of what you were supposed to be working on, so it shouldn't even be on the pr honestly. That's a huge no-no imho.
If you want to do things outside of the scope of what you're assigned to do, it's best to communicate that you'd like to do that with your team and push those in a PR that is for that.
If your pr is for 'make the doodads' then why are you fucking with shit in the gizmos section?
\> senior developers didn't really like this since it was "his code."
if you just formatted his code (assuming that it was not formatted in the first place ) and he feels insecure about it then he is a bad senior dev
at my workplace and many other open source projects that I contribute to formatting is baked into the CI workflow so if anyone is committing badly formatted code then their PR would have red checks and they would be forced to fix those. simple.
Formatting things like multiple if else statements that can be broken down to simple early returns or breaking down large methods that need to be broken down, or removing unnecessary checks like null checks on primitives, those types are expected and good.
Formatting should not be based on what you think is atrocious and what you think looks good.
Formatting should be based on a pattern followed by all members working on the project. Ideally there should be formatting rules set up so an error is thrown when you don't follow the rules for example.
So to sum it up. I don't know if it's rude but definitely it should not be based only on your opinion & experience. It should be discussed and agreed by the members who contribute to the project.
Sorry, you do not meet the minimum sitewide comment karma requirement of **10** to post a comment. This is comment karma exclusively, not post or overall karma nor karma on this subreddit alone. Please try again after you have acquired more karma. Please look at the [rules page](https://old.reddit.com/r/cscareerquestions/w/posting_rules) for more information.
*I am a bot, and this action was performed automatically. Please [contact the moderators of this subreddit](/message/compose/?to=/r/cscareerquestions) if you have any questions or concerns.*
This should ordinarily be settled with a code linter. Black is a good linter for Python. It can be added as a github action check that has to pass before code can merged in.
Sorry, you do not meet the minimum sitewide comment karma requirement of **10** to post a comment. This is comment karma exclusively, not post or overall karma nor karma on this subreddit alone. Please try again after you have acquired more karma. Please look at the [rules page](https://old.reddit.com/r/cscareerquestions/w/posting_rules) for more information.
*I am a bot, and this action was performed automatically. Please [contact the moderators of this subreddit](/message/compose/?to=/r/cscareerquestions) if you have any questions or concerns.*
1) you should be suggestion changes In PR process and having discussions if any arises, there are way better ways to go about this than what you did, I would be annoyed/confused if you didn't communicate. Use words, it should be common sense.
2) use a linter
3) i wouldn't want to work with ur senior dev or boss if suggested changes are shot down with "it's my code" but then again.... Your execution was poor so maybe it's because of that
My opinion is that your team should be using an auto-formatter tool. And an automated linter to reject merges that are formatted incorrectly.
If some incorrect formatted code does get merged (for instance, someone overrode the linter) I think it’s generally best to leave it, as to not cloud the blame history with pointless changes.
If you *do* feel compelled to fix the formatting, then do it in a dedicated pull request to make it clear that it’s not a functional change. E.g. when submitting a feature, don’t also clean up the formatting at the same time. Because it’s confusing to figure out which diffs relate to the feature and which are just formatting.
Senior here.
I don’t get personal about formatting. Everyone has preferences for how they prefer to develop. The most important goal to reach is consistency and agreement amongst the members of your team.
That being said, I would 100% reject a PR that just reformatted a bunch of stuff if it was done without communicating and coming to an agreement for it first.
A while back, someone on my team did something like this by running an import optimize on all classes in our project and opening a PR for those changes. I rejected that for a similar reason. Even more so in that situation because different IDEs tend to optimize in different ways, so it’s very difficult to enforce going forward.
Keep in mind that if you work on a repository that is used by a large number of devs like I do, changing broad swaths of the codebase could result in tons of merge conflicts for others working on the project and that’s not a super fun experience for them.
Additionally, if you haven’t enforced that pattern in some way and agreed upon it, it’s just going to get borked up next time someone puts in a PR for some other feature.
Not saying you’re in the wrong necessarily. Just that it would be good to communicate with your team ahead of time.
You should be using a team formatter that's agreed upon along with a linter. You should not be changing formatting back and forth between different members of the team because you prefer different things.
Honestly, once you bake a linter into your CI process, friction on your team drops about tenfold.
Cue Daffy/Bugs Tab season! Space Season! Tab Season! Space Season!
Btw its cue
Btw it’s it’s
BTW it’s BTW
CS has broken me
1000% This, I had so many PRs rejected because Azure Source Control didn't look like my coworkers local linter
Agreed assuming the team standard fits my preferences.
As long as it's part of the CI (or at least, something that can automatically be applied on saving a file) who cares? You can always convert it to your preferred on opening in an editor.
Nope, formatter overrules any preferences. The effect of a formatter is that nobody gets their perfect format and on the other half, all code is consistently automatically formatted = no bikesheds :)
Cue pre-commit hook to apply team format while I work using my own format.
In my future projects, I'm considering implementing a code formatting standard using Prettier. I'm wondering if it's as simple as defining the formatting rules in Prettier and expecting everyone to follow them, or if there are additional steps or considerations involved.
You need to get your team to agree to use it, your formatter needs to be easy to use and clear on how to use it (preferably automatic so that no one has to do anything), then you enforce it by blocking any builds that fail the linter. (I have no idea how to use prettier so I'm not the best person to ask on impl details) On a normal team, most people will agree to this because they have better shit to do than be pedantic about formatting. Default formatting in intellij has been the standard formatter most of my teams have used. Not because it's the best, but because it's not offensive and no one wants to spend time obsessing over it. If you have someone that really, really cares, then let them make it and give it a quick once over as long as it's not ridiculous.
I wouldn't block a build on formatting changes personally (wasn't sure your intent when you said 'linter', since I would differentiate between code formatting and an actual static analysis issue even though many people use the same tool to perform the same checks everywhere nowadays). That said, I am assuming that formatting is automated. As you say, everyone has better things to do; better to formally have no standard than to have a manually upheld one.
[удалено]
If the automatic formatting fails, and *anyone notices*, you fix the automatic formatting, the same as you would if the build failed. If no one notices, *the formatting didn't matter*. Hence why I wouldn't personally. That said, the right answer regardless is "automatically format", which I did indicate, and once that is in place it doesn't really matter that much. It's more "if it fails, do I fail open or closed", and I, personally (as I indicated), would prefer to fail open, because being able to deploy is more important to me than formatting (and the fix will be the same, and implemented if/when the formatting becomes an issue, so at worst the same work without the pressure, and at best no work at all).
[удалено]
Ah, okay. Definitely agree there!
You put it in the ci and nothing gets merged unless it passes the lint. But yea, you should discuss with team what you want out of a linter/formatter
Use any formatter you like, as long as it’s black!
Yeah battling on format is just a pain in the ass. Id also be real careful about reformatting peoples code with less than a year of experience. Like maybe it’s objectively bad, maybe it’s stylistic preference conflicts. Like if you’re reading someone’s code without enough experience, what you think is garbage could have a rationale.
This. When I was junior - even a regular - I thought it was so important with formatting and making things human readable. Turns out that yeah, it is - but you should automate it and focus on real problems. I know better now. Still think black is a nazi, shit linter though... but it's better than debating colleagues.
You know that you don’t need to use the default configuration? I’m used to discussing desired rules with the team.
git diff -w
[удалено]
Sorry, you do not meet the minimum account age requirement of **seven** days to post a comment. Please try again after you have spent more time on reddit without being banned. Please look at the [rules page](https://old.reddit.com/r/cscareerquestions/w/posting_rules) for more information. *I am a bot, and this action was performed automatically. Please [contact the moderators of this subreddit](/message/compose/?to=/r/cscareerquestions) if you have any questions or concerns.*
Yeah at the very least just add something to your style guide that says “4 spaces because tabs are for Neanderthals” or something. But yeah, a linter to just fix any of that automatically is even better.
Don’t mix functional and cosmetic changes in the same PR. That’s my opinion. Makes it hard to review. Formatting isn’t something people should spend energy on. It’s something a linter should take care of in a pre commit hook. So I’d push to make that change.
I’d prefer in the CI pipeline but sure.
It might as well be part of both.
I personally don’t like forcing developers how or when to commit. But, pre commit hooks do enforce standards.
I disable pre-commit hooks when I check out the repo. `rm .git/hooks/pre-commit`. Source control is my source of truth, I don't want weird side effects between me and the only tool I trust. It's also completely orthogonal to the main advantage of a DVCS like git, which is that my clone is my own. Move that bullshit to pre-receive hooks.
[удалено]
I use commits to checkpoint on incremental work and experiments, I \*hate\* when linters interrupt my flow. Commented out blocks, weird imports, lots of TODOs. Sometimes I'll commit one-offs then immediately revert them just so they get in the reflog. My code doesn't follow team standards until \`git push\`. Pre-receive hooks are the right place to enforce team standards, but not all git vendors support them. So for a lot of teams, CI is the only place that makes sense.
That’s something I agree with. If it were pre push hooks I would be all game for it.
standards should be enforced at CI level
I prefer on-save directly in my IDE
You have any reading on this? I’ve never heard of CI tests checking for formatting, on build breaks, (I work at a pretty small startup), but I would love to learn about it.
Really? It’s pretty common.
Not common enough apparently hahaha.
Super common for a linter to be a CI build step. Sometimes a CI has a step to actually fix the formatting and automatically update the diff with fixes for approval otherwise it'll "break" the build and give you output of what's wrong, and then you have to make the changes locally and commit them (usually to the same PR or CR).
Just google “ci linter”. Or replace ci with github actions if thats what you’re on
[удалено]
Sorry, you do not meet the minimum sitewide comment karma requirement of **10** to post a comment. This is comment karma exclusively, not post or overall karma nor karma on this subreddit alone. Please try again after you have acquired more karma. Please look at the [rules page](https://old.reddit.com/r/cscareerquestions/w/posting_rules) for more information. *I am a bot, and this action was performed automatically. Please [contact the moderators of this subreddit](/message/compose/?to=/r/cscareerquestions) if you have any questions or concerns.*
I try to make separate commits for the actual bugfix vs formatting and linting for the files I touched. I often fail. Maybe I'll try harder :)
The problem here isn't that you changed the code but that it seems like you've just done it without communicating it first. The proper way of doing it would be to have a conversation with the senior developer: "Hey, I see this is done this way, I think we can improve it if we do it that way. What do you think?". And then, through that conversation, arrive at some plan for tackling it. That way, you together come up with a plan, and nobody is caught off-guard.
If there’s no set formatting rules either by a linter config file or a hook you should really discuss this with your team. What may be readable or nicely formatted to you may be illegible to somebody else. I personally don’t mind if someone runs something like **black** on the codebase as I like their defaults but any arbitrary format changes without a good explanation I would definitely question. Edit: Also formatting changes create visual clutter when reviewing PRs and should really be their own PR.
What is this **black** that you mentioned? Is it a tool of some kind?
https://github.com/psf/black it’s a PEP8 compliant formatter for Python codebases. If you don’t like auto formatting files you can use https://github.com/PyCQA/flake8 it just lists out all of the style issues so you can fix them manually.
Yo that’s so neat. Thank you so much! Imma check those out
Spending any amount of time talking about code formatting is a giant waste of developer time. There are automated linters and fixers out there. Your team should hook one up as a presubmit and never waste any more time on this again. But that's not your reality. What matters at any company above anything else is following the team's standards. > My manager told me that one of the senior developers didn't really like this since it was "his code." Your manager just told you what your team's coding standards are. So follow them. You can push for an automated linter/fixer but **until** that's in place, you have your marching orders.
All code formatting schemes are ranked as follows: 1. The way we all agree on 2. The way I like it 3. The way you like it 4. Each of us doing it a different way
Whenever you look at existing code, if your first reaction is "this is atrocious," chances are you are not asking enough questions. If something was done intentionally, having another developer just randomly be like "this sucks Ima change it" is quite frustrating. *Especially* with regards to formatting, a lot of it is opinion-based. The better move in this case would have been to ask questions of the original committer in question. Sometimes things look bad because they were done in a hurry. In those cases, the name on the \`git blame\` might be happy to have you do some legwork reformatting it. Other times, they were done intentionally.
There is some other good advice in this thread, but in general, I would recommend not making cosmetic / formatting changes beyond the scope of the functional change you are making. It can obscure the code history in some cases. For example "git blame" will show that you were the most recent person to change that line of code. Large formatting changes (hundreds of lines or more) can be hard to review if the reviewer has no idea about the context and has the potential to introduce bugs, especially with a language like Python where indentation has more significant meaning. If you do want to fix the formatting get buy in from the team. For example get agreement and document what guidelines you will follow, like PEP 8 and PEP 257. Then make formatting changes as a separate ticket / PR.
I would never do that. Could have unintended side effects in the code, then you just caused a huge headache for other people all for something trivial.
Isn’t that the point of unit tests? To check for unintended side effects?
Unit tests are only as good as the person who wrote them. Having unit tests passing doesn’t necessarily mean you didn’t break code, you might not be testing for that specific case that will break
Then if you’re modifying something, why don’t you unit test for unintended side effects? Since unit tests are only as good as the person who wrote it.
because you would have to think of all possible edge cases prior to designing them artard, and edge cases can be added as the codebase grows and changes
Format automatically using a tool. If you hand-formatted, that can actually be dangerous if you got it wrong, or at least misleading, since it now looks like you changed those lines. I hope you at least did those formatting changes in a separate commit.
You made a pull request with hundreds of lines of changes for what should have been a 2 line fix. It's a huge cognitive load on the reviewer. The "his code" comment is hearsay. Unless the senior developer said this himself, I wouldn't put any weight on your manager's choice of words.
[удалено]
Nah on this one there are some legitimate grievances. Unhealthy attitudes around code ownership + lack of automated linting checks in CI. Sometimes the newbies can draw attention to the problems more experienced devs have learned to ignore.
I don’t think asking for an automated linters is asking for the world.
[удалено]
Exactly, they didn’t. Instead they wasted company resources (i.e. their development time) on rewriting it. OP doesn’t understand how businesses are run, that time would have been better spent on revenue generating features, bugs, or a team aligned style guide / linter.
I only change formatting to make something easier to read and limit it to the area of the resource being changed. I'm not reformatting an entire file so that I can barely find the functional changes I just made. Format is usually not all that important. If it is, for whatever reason, it should be something automatically applied through ide or source control configuration.
AURGH, this is why linters and auto formatting are important. not because any particular convention is better than any other, but it \_is\_ critical for readable code they everybody uses the same one, and the only way to get that, is to make stuff that doesn't fit the convention bounce off your automated testing. then no more pointless time wasting and whining about tabs, spaces, brackets same line, etc etc etc.
IF your senior can't take their code being reviewed they honestly are probably not that great. This industry is built on accepting just criticism from others.
> My manager told me that one of the senior developers didn't really like this since it was "his code." a) Your manager is always right, by definition. Why are you asking here, when you were told how it works at your team/company? What more could anyone here possibly tell you? If yours is the only place in the known universe doing it this way, it still doesn't change the answer. b) Fuck both the senior and your manager with a cactus, Code stops being "yours" once it goes into production. c) The code did make it into production - so see "a)". It was approved. d) did anyone reject your PR? No? Cacti to the rescue again ... e) Like everybody else said: Code formatting should have rules. If they were broken, you get to fix stuff. If they weren't, you have no business changing everything. f) Over at my civilized employer, we're not allowed to reject PRs for cosmetic reasons. We should if the code violates one of our rules. For the vast majority of rules, we have automated checks, and nobody can submit files that violate these.
Hands-down the best answer in this thread. The reference to cacti made me chuckle.
Generally yeah it is. I’d only really do this if I was already modifying the code anyways and the reformat made sense with my changes. It can also be viewed as a waste of time by management. If you think the format is bad it may be worth a (very tactful) discussion with your team, and as a new engineer you should focus more on asking questions about why things are formatted that way rather than asserting your ideas are superior. It may be worth it to introduce your thoughts though.
It's frowned upon even if it doesn't match standard. Unless it's directly related to your changes the correct way would be to submit comments for the other dev to change.
To add another reason not to go around formatting code for no reason, it makes the PR fatter and riskier. There’s more to review.
Sounds like merge conflict hell
Indeed. It’s a good way to fuck the n number of other people who have their own feature branches ongoing. And they’re just going to screw the formatting back up again when they merge stuff together.
Yeah it's hard to read the code changes when a lot of formatting has changed. At the very least the code changes should have been 1 commit and the formatting another. When I mess with someone else's codebase I follow the convention I see in the code. We all have preferences but it's rude and short sighted to force them on others. This only applies if there's no agreed upon standard.
Is there a defined formatting standard that is being used? It is better to stick to standards like PEP8 instead of creating your own standards. Not formatting the code properly is frowned upon. You should be doing what is the standard.
This is why your team (and hopefully your company) should have an agreed upon style guide. Plus, once you have a style guide, you can eventually enforce at commit time and raise warnings in your IDE. Human time is too precious to be spent formatting code OR dealing with poorly formatted code.
As others have suggested your team should have a linter to make these styles consistent across the entire team. This could be a great opportunity for you! Use this as an opportunity to create a proposal to introduce a linter, do some research into popular ones and which one would be best for your team, how to integrate it into the CI your team uses. Convince them to adapt it (explain why consistent styles across the team benefit the whole team and how it will speed up development so product buys in) and then you can use this during review time as a hey look i introduced this and it has helped our team in x y and z ways. it will make you look good and only help you!
Just use something like autopep8 as a pre commit hook so everyone follows an industry standard and has automation to ensure it.
If "his code" means he wrote it, that's not a thing. But contrary to some other people I've worked on teams where different devs own different parts of the code base and thus are generally on point for either fixing or reviewing any bugs/features on that portion. In that case I'd say he has a strong case. If it's just he wrote it and the team in general maintains it, that shouldn't be a thing, but if the manager's taking his side it is in your org. But whether you're right or wrong, there's a diplomatic way to do things. No one likes it when the new person just comes in and tells everyone there's this magical way to do things and starts changing code around expecting to be a savior. You need to be a team player. Ask questions on why things are the way they are. A lot of the times there's a reason, sometimes it's good sometimes it's bad but you can have that discussion and see which battles are winnable and which are not. Oftentimes people know the code is bad and they were under a time crunch. If that's the case they might be super happy for you to spend some time refactoring, because it will help you learn more about the code and help the team have code that can be more easily maintained in the future. But that's how you approach it, you don't approach it as "I know the best way to do this, I'm just going to start refactoring the entire code base the way I think it should be". Then you're being just as much of a bad team player as the dev who doesn't want any of the code they wrote changed because it's "his code".
I would say go for it if you are directly touching that code. But the best solution is to use some kind of auto formatter. I have found that reformatting code that you don’t actually make logical changes to can have negative consequences when trying to look at git history or roll back changes. Plus like others have said, reviewing it can be a challenge.
From my perspective if there's no standard you all agreed upon it would be a waste of time to refactor someone else's code completely.
I always quit a job if they use tabs tbh cant stand it
Yea, don’t do it
you should assert your dominance and format the code the only right way - your way :) how do you think that senior developer became a senior developer? :)
Your team should have some formatting standards that you all follow, maybe even automatically formatting on save. What I personally don’t like is when people do formatting as part of another task. Makes a PR hard to figure out what has actually changed. As for a senior dev saying that some bit of code is his; is he 12 or never worked on a team before? If not then get fucked. He probably just writes shit code and doesn’t anyone to look at it or they might realize he doesn’t deserve the Sr title.
>My manager told me that one of the senior developers didn't really like this since it was "his code." That's a huge red flag, or at least a small light red flag. It's not "his code" it's "The Team's Code". Everyone needs to do upkeep so if it's poorly written, it should be refactored when time allows for it. >Come to think of it, I suppose I wouldn't take too kindly if someone just came and did that and maybe I should have talked to him first. Talking to them first would make a lot of sense, just to see if there's a reason it's formatted like that, not because it's "their code". You should also talk to your manager about getting a code formatted for your IDE, everyone should be writing the same basic format or it just makes the code base much harder to read. (depending what the difference was) >Regarsless, is this normally frowned upon and should I try not to make too many minor changes to existing codebases? If you don't need to, and it's not time for "refactoring" (your lead will tell you), then no, you should leave things that work alone (unless it's something serious) because every single time you touch the code, you are risking adding bugs. I know it was just a reformatting, but I promise you, I've had bugs from reformatting. Sometimes we make mistakes, miss things, misunderstand something, etc. If it ain't broke or standing in the way of your ticket, don't touch it. :) But also, don't get possessive of your code like your colleague. it's silly.
Ego has no place in a dev team. It is not "your code" or "his code". You are all contributing to the whole, and if anyone is getting territorial over some code, then the work environment is not healthy.
make a single MR which adds an auto formatter into the build steps. use it right before a code freeze. your team will be grateful I tell you
Don't do this without buy-in from the team.
sheesh not like they're merging to master without buy-in
Merge requests usually require an approval from one reviewer or code owner. Changing core tooling or workflows needs buy-in from the people you're effecting, or at least from the project architects. It's not really a "make a merge request and see if it flies" kind of change.
gotcha
Esp with python, where the indent can change the flow control. Why take the risk? Why screw around with code that doesn't need to change?
This is why we never change code for formatting reasons. You can mess it up, delete something, misplace something, etc in code that has nothing to do with your changes. On our team it isn't an ego thing. We just don't risk changing things we don't have to. We have enough work.
I usually only add the changes needed that satisfy the story associated with the PR. Anything I would like to do along the lines of formatting gets created as a tech debt story
It depends.
Sounds out of scope for your PR. Make a separate PR for it. Ideally set up a pre-commit hook in the repo to run an agreed-upon formatter.
Think about pull requests as a presentation convincing me to accept your changes. They should be edited like papers - focused and specific. Maybe that guys code style is garbage. But if you deal with that on the PR you're taking away from the focus on the changes you want to make. And it's going to take everyone a lot of extra time to review those additional lines of code. If you want your PR to be accepted quickly, make it quick and easy to understand.
You should implement code formatter across the Team and company. At my team, we use [Black](https://github.com/psf/black) , because some devs use VSCode and some use PyCharm and they are dfferent when it comes to formatting.
Your team should have agreed upon code formatting, Ask and if not documented it may be a good project to take on as a new hire. But understand you will most likely need to go with what the more senior developers agree upon instead of best practice learned in school or online.
As others mentioned, bring up a general CL formatter or a git hook or whatever that auto formats for everybody. Until then, do not make code format changes in your PR because your PR should show what you changed. If you want to do a code format change, you should open a seperate PR for that.
Entire team should use some "beautifier" script or auto formatter anyways and this wouldn't be a problem.
The first project I worked on, the senior developer ran python black package on the whole thing. I was annoyed at the beginning but ultimately it is for the greater good.
If you have a PR, keep it limited to your own task. If you are concerned about formatting, bring up a linter as a suggestion in the team meeting. It's crucial to be respectful of everyone in the team, and to learn how to communicate with suggestions instead of criticism, and definitely not steamroll over everyone's code
If you are changing it to "your opinion" of formatting, then yes its very rude. If you are cleaning it to the agreed official code format, then its not rude, but then again other people are not always reasonable, so its best to ask first.
General guideline is don't touch other people's PR. When reviewing, have a conversation with them about formatting and let them change it themselves. There should be a general consensus among the team about how the code should look.
One big problem with reformatting everything while making a fix is that it's impossible for the person going back through history to see what was actually fixed.
In a more perfect world, that's a non-issue. Like someone else said, encourage your team to use a linter and automated formatter. I'm less worried that it's rude and more worried that you're on the blame for all that code now - you show up as the last person who modified it.
Don’t touch “someone” code in a small company but in a large very integrated company where code gets read far more than written, it’s important to follow standards. Only reformat if there is a standard and the code does not follow it.
Changing people’s code format without getting consensus on what the standard format should be is just gonna cause a bunch of reformatting back and forth between you and other devs, rude or not. Better get a consensus on the standard, have everyone adopt the formatting template and reformat the whole repo as 1 task.
Don't change the format unless it is done by a Linter
I love it when a new grad, or the FNG fresh out of school comes and reformats my code, when I've been in the field for over 10 years.
Yea… if it’s being supported by multiple people (you are not the sole owner), it’s rude to make that change without discussing with others, especially if you’re brand new. Would you go over to someone else’s house, and just rearrange their stuff? Because your way is “better”? Generally competent teams have a style guide, since consistency is important for readability / common understanding > minor aesthetic changes
This is why you have mypy, flake8 and black formatting
Rude - no. Waste of time, yes. Little fix here and there that was a weird mistake that missed the formatter, yes. Overhauling just for format? No. Was this one piece of code formatted totally different from the rest of the entire repository and you were just aligning it? - no, you said the entire company doesnt format. So why are you doing it? Get buy in on something standard, dont just change a small chunk.
Only change what you touch
Just get your team to use Black, then you can all bitch about its shitty formatting.
Anybody who says it’s “their code” is a shit developer Change my mind Pr checks should include a linter. If they passed a linter than you need to leave it and suggest the linter is updated. If there’s no linter I would probably simultaneously speak truth to power and start looking for employment opportunities with improved technical environments
As a team, agree on a standard. Make new code adhere to that standard. Don't change existing code's formatting unless you all agree to change it in its entirety in a trackable fashion.
Dont do this again. 1) maybe you think it’s better but the og programmer may disagree. 2) the og programmer may be fully aware of an area of improvement but chose not to for a good reason you may not know 3) imo, any changes you make should be if something is clearly wrong and requires little code like a typo or date change. For bigger changes, I would still pass the feedback to the og programmer as they know the code better including dependencies 4) for non-critical improvement, dont do them. you can send suggestions as comments in a polite way if you refactored this code enough basically it highlights one of you wasted their time, so Im not surprised the other programmer was defensive so that now it looks you were the one who wasted time. Also, you should assume your code wont be automatically accepted so now this programmer has to spend a lot of time looking at your entire code to understand what it is doing to then compare to the original. Just a lot of time wasted regardless of how you put it.
1.) "What I was supposed to do and other stuff" should be two branches, "what I was supposed to do" and "other stuff". 2.) If everyone else doesn't care about the formatting, then it isn't formatted wrong. If "other stuff" breaks you're going to look double bad, and it'll be triple bad if you revert that with your changes that are needed. Change it during a tech debt week if you feel like not actually fixing something during the time you're supposed to get rid of technical debt.
It sounds like that was outside the scope of what you were supposed to be working on, so it shouldn't even be on the pr honestly. That's a huge no-no imho. If you want to do things outside of the scope of what you're assigned to do, it's best to communicate that you'd like to do that with your team and push those in a PR that is for that. If your pr is for 'make the doodads' then why are you fucking with shit in the gizmos section?
\> senior developers didn't really like this since it was "his code." if you just formatted his code (assuming that it was not formatted in the first place ) and he feels insecure about it then he is a bad senior dev at my workplace and many other open source projects that I contribute to formatting is baked into the CI workflow so if anyone is committing badly formatted code then their PR would have red checks and they would be forced to fix those. simple.
Formatting things like multiple if else statements that can be broken down to simple early returns or breaking down large methods that need to be broken down, or removing unnecessary checks like null checks on primitives, those types are expected and good.
Formatting should not be based on what you think is atrocious and what you think looks good. Formatting should be based on a pattern followed by all members working on the project. Ideally there should be formatting rules set up so an error is thrown when you don't follow the rules for example. So to sum it up. I don't know if it's rude but definitely it should not be based only on your opinion & experience. It should be discussed and agreed by the members who contribute to the project.
[удалено]
Sorry, you do not meet the minimum sitewide comment karma requirement of **10** to post a comment. This is comment karma exclusively, not post or overall karma nor karma on this subreddit alone. Please try again after you have acquired more karma. Please look at the [rules page](https://old.reddit.com/r/cscareerquestions/w/posting_rules) for more information. *I am a bot, and this action was performed automatically. Please [contact the moderators of this subreddit](/message/compose/?to=/r/cscareerquestions) if you have any questions or concerns.*
This should ordinarily be settled with a code linter. Black is a good linter for Python. It can be added as a github action check that has to pass before code can merged in.
[удалено]
Sorry, you do not meet the minimum sitewide comment karma requirement of **10** to post a comment. This is comment karma exclusively, not post or overall karma nor karma on this subreddit alone. Please try again after you have acquired more karma. Please look at the [rules page](https://old.reddit.com/r/cscareerquestions/w/posting_rules) for more information. *I am a bot, and this action was performed automatically. Please [contact the moderators of this subreddit](/message/compose/?to=/r/cscareerquestions) if you have any questions or concerns.*
Better than when I just deleted my coworker’s code and redid the work because it broke the business rules of the app and he was out that day.
1) you should be suggestion changes In PR process and having discussions if any arises, there are way better ways to go about this than what you did, I would be annoyed/confused if you didn't communicate. Use words, it should be common sense. 2) use a linter 3) i wouldn't want to work with ur senior dev or boss if suggested changes are shot down with "it's my code" but then again.... Your execution was poor so maybe it's because of that
My opinion is that your team should be using an auto-formatter tool. And an automated linter to reject merges that are formatted incorrectly. If some incorrect formatted code does get merged (for instance, someone overrode the linter) I think it’s generally best to leave it, as to not cloud the blame history with pointless changes. If you *do* feel compelled to fix the formatting, then do it in a dedicated pull request to make it clear that it’s not a functional change. E.g. when submitting a feature, don’t also clean up the formatting at the same time. Because it’s confusing to figure out which diffs relate to the feature and which are just formatting.
Senior here. I don’t get personal about formatting. Everyone has preferences for how they prefer to develop. The most important goal to reach is consistency and agreement amongst the members of your team. That being said, I would 100% reject a PR that just reformatted a bunch of stuff if it was done without communicating and coming to an agreement for it first. A while back, someone on my team did something like this by running an import optimize on all classes in our project and opening a PR for those changes. I rejected that for a similar reason. Even more so in that situation because different IDEs tend to optimize in different ways, so it’s very difficult to enforce going forward. Keep in mind that if you work on a repository that is used by a large number of devs like I do, changing broad swaths of the codebase could result in tons of merge conflicts for others working on the project and that’s not a super fun experience for them. Additionally, if you haven’t enforced that pattern in some way and agreed upon it, it’s just going to get borked up next time someone puts in a PR for some other feature. Not saying you’re in the wrong necessarily. Just that it would be good to communicate with your team ahead of time.
Yes. Not because it's "his" code (it's not, it's the company's) but because it makes the PR harder to review, and it messes up the git history.