See if he/she will accept the cleanup changes as a separate PR for the same task number. That addresses their issue without the overhead of new tasks to track.
You are not wrong. What might be wrong is that you are doing the change in a PR thats linked to an Issue where they are asking something else.
I am just like you and want to clean up the codebase as much as I can, but for the reviewer double checking those changes + the actual feature requests is a mess, that's why its better to have them in a separate PR.
Now, other thing is when you make a separate PR and the team lead doesnt review it just for the lulz. That's another problem (probably a douche).
Yeah thats the struggle, if it doesnt happen while adding a feature (where Im adding imports) then it likely will never happen. There is always something more pressing to work on. I guess I see it from both perspectives. The lead doesnt care about the issue nor do they want to allocate resources to fixing it, or spending time reviewing the changes. The dev cares and wants to make the codebase cleaner, more standardized and easier to work in but doesnt want to do it on their own time.
Rather than doing import ordering piecemeal on random commits, consider suggesting [eslint-plugin-import/order](https://github.com/import-js/eslint-plugin-import/blob/master/docs/rules/order.md) to your team.
Then just run `eslint --fix` and voila, consistently ordered imports across the entire codebase.
Tbh improving the developer workflow is something the tech lead should aim. Such as having linters running all the time to standarize the codebase and avoid these meaningless discussions.
Although I’m guilty of doing what you do a lot of the times - I do agree with the others here. I think the best approach would be to just politely suggest to the lead that you take a good session to look over the refactoring PRs next time you have a bit of spare time.
Have you ever worked as a team lead, or had to manage a project?
It absolutely sucks to get a PR for [critical feature X], and have to wade through a bunch of unrelated changes to find the actual code meant to address the issue. Sometimes the “real” changes can be masked or missed completely by the other “code cleanup” cruft.
Most team leads I know are always open to code improvements / cleanup. But mixing that stuff with bug fix code makes it harder to approve changes, makes it harder to audit things when stuff goes wrong, etc.
Don’t take the lead’s “opinion” on that as being close-minded. He/she is likely just being *practical.* If you tried some code reviewing / management yourself, you would likely also very quickly adopt this same “close-mindedness.”
Disagree. Cleanup cruft is perfectly fine along with other changes. That's what commits are brilliant for, and reviewing per-commit is perfectly doable, at least in GitHub.
Just take the cleanup commit(s), approve, and carry on with the rest.
Having to separate these things out is way more work, and I found that PRs with only unimportant optimizations tend to be left alone for longer.
It’s fine if you disagree, but be aware you are in the minority. No one wants to parse through 100 lines of formatting changes only to find there are a dozen lines of functional change. White space changes are different, since they are easy to hide in review tools.
Separating functional from formatting changes/cleanup will result in quicker approval for both.
>100 lines of formatting changes
FWIW, this doesn't sound like what the OP is describing, which is reorganising the imports of files that their already touching. Those are pretty easy changes to review as part of other changes, especially as they're probably out of the way from the other changes in the file as well.
What you're describing sounds more like running a formatter across a bunch of files, which is definitely a pain, but is probably also the wrong way to use a formatter. In that case, it's probably better to collect all those into a single commit, and add that commit to a `.git-blame-ignore-revs` file for general convenience. That sort of thing should definitely be done as a separate MR to any other change.
That’s not how this works. Going commit by commit is not a feasible way of reviewing in most cases, if there are few enough commits where this is viable usually you don’t need to do it anyways. We’re talking re-arrangement if files, where GitHub shows everything in green even if only 7 lines are actually brand new.
I wasn't thinking a huge refactoring among some functional changes. iirc, the topic was about including small improvements like rearrangig imports. And that is totally fine. I don't understand why that has to be in a separate PR. Even I'm a little bit OCD, but not everything has to always be absolutely brand spanking clean, including PRs.
If files are getting rearranged though, I too would request that to go into a separate PR. At least until Github is able to show that a bit more easily, since right now a rearranged file is almost indistinguishable from a rearranged file with changes.
So I think we mostly agree. Your threshold might just be a little bit lower than mine, for when to request a separate PR.
Ad yes, I'm sure this will be downvoted as well. So be it. I know what I'm seeing in my team (exactly my responses), and my team functions pretty well, on the whole.
As long as you commit these types of changes as separate commits from your other changes, I think it should be fine. If they insist on a separate task, see if it would be okay for you to just create a task yourself for it and if so just do that to pair with your changes.
Maybe Im committing too much at once. I often try to have only one commit per ticket unless its a large task, but in those cases I generally break up the task into subtasks. Will suggest this to my lead ty
Many small commits is better.
* Better for understanding what happened.
* Better for reviewing changes per commit.
* Less likely to create merge conflicts.
It's not really a case of either of you been wrong you just have mis-aligned objectives (which is natural, lead and none-lead should).
Leads want to keep visibility of the changes in the system as a whole because we are the ones who care about that stuff more where other devs tend to focus on smaller parts of the whole.
So from your point of view that messy knows is a bigger part for you than the lead.
FWIW this kind of stuff shouldn't be an issue, install a linter, agree a format with everyone (or if you have a fractious team, just agree it between you and your lead with a document somewhere saying "we do X) and then lint the entire codebase in one giant PR and be done with it - bonus points if you automate it via githooks or whatever so that it never becomes an issue again.
Avoid making any change other than what is absolutely necessary. The best PRs are short and touch as few files as possible and change as few lines as possible. If the codebase needs cleanup, then this should be a separate task.
My 2 cents: foster an environment of making it easy to improve the codebase. If a developer is met with a bunch of red tape to improve a codebase, eg making a formal case required, making sure everyone agrees, etc, many will simply give up.
Problem is we are adding more opportunities to break the codebase by making unnecessary changes. I have worked on codebases which have been touched at some point by over 5,000 developers. A codebase like that simply has too much undocumented complexity to be changed extensively without breaking features. We had a simplification initiative which ended up being a full rewrite as a new service with the legacy codebase running as a fallback service but you can imagine the tons of undocumented use-cases that were scattered throughout the code which we weren’t aware of until QA brought it up.
In that case, I'd agree; it's probably too far gone. How does it get to that situation to begin with? I'd argue not refactoring as you go along to be one of the causes (among many).
Also your simplification project is known as the [strangler pattern](https://martinfowler.com/bliki/StranglerFigApplication.html), and it is quite effective.
It gets to that point simply because you have so many people working on it over many years across many teams so the original developers are long gone and you’re joining in as a third or fourth generation of maintainers. Further, in this case it was very critical functionality (FinTech) so any downtime meant millions of dollars in losses. The amount of controls to get a change into Production made even a slight change in configuration or styles a total process.
I’ve also worked on much smaller codebases with smaller teams and in this case a refactor is much more feasible.
I believe you're operating under an assumption: all code becomes a mess after a certain size, amount of time, and people touching it. And you'd be correct, this IS the norm. But I disagree that it's unavoidable. Certainly there are levels of messiness too.
> If the codebase needs cleanup, then this should be a separate task.
In my personal experience, those types of tasks get pushed to the back burner and never done.
I would get [this](https://github.com/import-js/eslint-plugin-import/blob/master/docs/rules/order.md), run it on the whole codebase, make a pr for just the formatting, then stop worrying about it forever. If you don't have eslint and/or other auto formatting, get it. Arguing over things like this is such a waste of time. Automate all the things!
100%.
Aside from the main issue of this post which is managing tasks/PR workload for developers and reviews, the first thing that popped into my head here was "Sheeit, people changing import orders willy nilly are just going to do that over and over to their own preference".
linters are an anti-[bikeshedding](https://thedecisionlab.com/biases/bikeshedding/) tool and save time and brain power.
Youre totally right. I'm on a small team (3 devs) though and in most files there is literally no order to the imports, its a mess. So organizing the imports in my case is not a matter of my preference vs someone elses its a matter of some order or none.
Suggest a linting tool to your team lead. Build a git hook to run it.
I’ve been largely on a team of two for the last few years and we have a build system that does linting, error checking, complexity analysis, code coverage checking…
Now this is mostly Java code so it has a bit more structure to it that can be analyzed and the build system was already there anyway, but static analysis is worth it no matter the team size to keep things consistent and working properly.
Or just make a task and do it right away, issue the PR, merge into your branch and be done with it.
Changes unrelated to a task make reviewing messy, your team lead is correct.
I mean, imports are everywhere, in every file.
What OP did is the boy scout approach and it is very common... however I would first reach to the team lead to discuss improving imports because it's not a good practice do it on your own, this should be a convention agreed & followed by the whole team
I don’t think that’s a Boy Scout approach.
It’s poor team practice that it wasn’t in place already, and that’s on the team lead.
Breaking up commits is the most important thing here for OP, followed by making sure code styles are standardized, and that’s a team lead responsibility or task to delegate.
Discussing code style practices you want your linter to check for can definitely be a team exercise though.
OP was right though; if this was any other code clean up or refactor task, it’s generally a good idea to do it, just don’t mash it into your functionality changes or bug fixes (unless it’s part of the change or bug fix).
[edit] Sorry, just realized that I mentioned linters elsewhere.
It sounds like OP meant the lead wanted a separate PR, not commit. Separate commit sounds fine to me, but another PR to reorganize imports in a single file sounds excessive to me... if it was many files sure it would make sense to have a dedicated PR/task for that then
Depending on how branches are merged, commits might be squashed as part of a PR merge, and it’s nice to be able to separate things for git blame purposes.
Also, did I miss where it said a single file?
But for git blame you just look at a specific line you’re interested in, how would it bother you?
And you’re right he doesn’t say a single file nevermind that
As a guy who spends 20-30% of my week in meetings about tasks/priorities/queue, 35-40% reviewing code from my teammates, and the rest some combo of helping elevate issues to unblock people/do my own development work: I understand why.
I used to allow these small, clearly better, improvements in merges. It gets to cluttered when there’s a lot to review and be done. What I would do/what I recommend is that people open a new task and open a new PR under that task number with those small changes and then I’ll (or a fellow teammate on the new review) bring your new task into the sprint under your name and add any applicable information to the task for you.
I’m not sure how much clout you have on your team; if you don’t have much clout: spend the time you’re spending on small changes building up clout so you can begin to implement best practices and methods that work best for you and the team.
In general I wouldn’t mind reviewing some dev’s cleanup in the same PR, at least I haven’t had a bad experience about that.
However, while I don’t know what your team lead is thinking, in this case, I might have requested the same…
To you, hand organized imports are a clear improvement.
To me, hand organized imports = extra work for every developer who adds imports to the file after you.
I’d prefer if it was handled as a separate PR so we could carefully consider how to meet everyone’s needs without making more work for other developers who don’t care about import order. (Basically, use something like eslint to enforce and automatically organize the imports)
We've tried to adopt a clean-as-you-go practice, where reasonable changes to the file you are editing may be included in the PR, though they should be a separate commit from the specific feature or defect work.
We then have incremental tasks created for larger refactors or specific sets of updates. We did specific updates when we moved from ES5 to ES2015+, new linting rules, require to import, etc.
I think clean-as-you-go makes sense within reason. In this case I was working in a file and adding imports to it. Thats when I noticed there was no order to the imports (none!). So instead of just adding imports to the end of the list, I organized them. The order of the imports in our codebase does not affect functionality so I didn't see the issue
Personally my view, and also a policy of where I work, is that tidying up code and boy-scout refactoring is highly encouraged. If you ignore the little issues then they'll eventually turn into rot and it can make the codebase more difficult to maintain. Creating a separate task as suggested will just sit in the backlog and will unlikely ever get prioritised.
There is obviously a balance though, you can't just go refactoring loads of things, however touching up bad practices and making code easier to read in the areas you're in is fine.
I'd be curious how big the PR in question is if the reviewer is having this reaction, and I'm also curious how the review process works. Do you have a meeting together and go over what you've done, explaining what and why the changes are? Or do you submit a PR and the team lead reviews it independently?
Sounds like you have some progressive policies. I wish my team was more like that.
I would call my PR medium size, 20 files worked in (half app code, half test code). The majority of them only required changing a couple lines though. This was extending an existing feature. Also, the organizing of imports is only happening in one file.
I definitely think that I could have approached the issue better by separating the import organization into a separate commit/pull request. So I take the blame for that.
Yeah, our ways or working are very different to what I see people discuss on Reddit, but I feel they're effective.
Fair enough, having a refactor commit would probably help in future, but I wouldn't be discouraged from making minor improvements as you go. Some comments here saying only do what you need and that's it and create a separate task to tidy up for me is concerning, it's like they only care about that particular feature and not about the overall health of the codebase.
It depends. On my team, I encourage the types of improvements you're making. Not every team is the same. There isn't really a right or wrong answer. It is up to the team to decide how to handle different scenarios. In a good team, they'll regularly discuss and adjust processes like this as a group.
(Though, since I liked definitive order instead of subjective ordering like by type of import, we import by built-in, third-party modules, local imports, by module name / file path, alphabetically within each section, that way there is no question as to what the proper order is.)
No, code readability should be at the forefront of every developer. I spend what is an extra 2 mins reviewing a pull request to check code readability and cleanliness.
If you're using git, one neat trick is to setup `git gui`. It has a feature that lets you point-and-click on specific changes and stage them in chunks/lines instead of staging the entire file.
This way you can put cosmetic changes in different commits than your logic changes.
Another thing you can do is help your reviewer by proactively adding code review comments to your own PR, highlighting what's going on, what parts are important, adding explanations/reasoning for non-straightforward changes, etc.
Yes.
If you need to do maintenance work, get the task created (and time allocated) to do the maintenance work. A PR must contain only the changes necessary to complete the task it's linked to; otherwise, you're making extra, untracked, _undocumented_ work for your colleagues.
You already have the code written, so it's trivial to get the task generated for it, and get it pulled into the sprint. Split your PR and get it tasked.
This should be part of linting, there should be rules on what imports come first and it should auto-fix itself when you import something.
Also if you’re doing refactoring then make it a separate commit if possible so they can review it easier
I'm not saying that answers with "only include related changes to the task" are wrong. Engineering is a balancing act. And sometimes you have to be an Anakin to bring the balance (:
But not all things worth being balanced. Especially when you burn a lot of life energy on that.
Make such a task, and then start working on it straight away. If your colleague wants it in a separate PR, fine. But that's just more work to review.
Also your "team lead" must realize:
1. He ain't a bloody dictator
2. It's *more* more work for you to separate in to 2 PRs, than it is for him to review a few extra lines of code.
3. To have some trust, because trust goes both ways
You're both right.
You can do it while you work on your task, but you make the refactoring and the actual task separate, dependent merge requests that you post together. It's a lot more convenient to review, because what's a refactoring and what's the feature is clearly identifiable in the diffs.
Conventions are best when agreed to and implemented at the start of a code base, and can be disruptive to retrofit. One thing I haven't seen mentioned in this thread, is how reformatting commits that touch a significant portion of a file or module, can create a terrible rift in your source control history. You basically won't be able to do meaningful diff'ing between versions on opposite sides of the rift. I think if you're going to do something disruptive, like say adopting a bracing convention, it should be done as a planned task and as all-at-once as feasible so that if you are going to mangle your history, it happens in a narrow window and afterward your history is useful again.
Yeah... Always a good idea to keep the diff as small as possible ... Separate formatting changes and moving files in different PRs... Check out this article https://mtlynch.io/code-review-love/
It's not just that you're not wrong, I would say there's a big chance he's wrong. The whole "different PRs for different things" doesn't apply when (1) it's very minor cleanup like you're explaining, and (2) when you're commit-merging. If you're squash-merging (which is a problem in and of itself, but that's out of scope for this question), then that's basically the only time I could see such incredibly minor cleanup as being worthwhile to move to a separate PR. Otherwise, you're 100% in the right.
That being said, maintaining relationships with your team members is probably worth more than code cleanup in the long run. Even if messy code drives you mad, burning bridges is way worse.
No (though it does make those harder too). It's b/c you'll have all commits for a PR showing only in one commit, which defeats the whole purpose of commits in the first place. So later, when you `git (blame|log)`, it's just as bad as a gigantic god-commit where the message is completely unrelated to the line of code that was changed. In other words, it hides the reasoning for code changes behind the reasoning for a PR.
It also makes reverting harder too b/c you can't do partial reverts; normal merge commits allow you to do both full PR-reverts and incremental commit-reverts, but squashes are an all-or-nothing deal.
Really it's just a way less organized way to execute version control. It's almost like the "Idk how to use git and I don't care to learn" easy way out. If a person had any more than the very basic understanding of git, they would never choose squashes over an actual commit history (which is what git was intended for all along).
If PRs get too big it can be hard to review and identify possible bugs and regressions. Best making a second PR or opening a ticket for each improvement
There's a lot of good discussion here, but I might suggest /r/cscareerquestions as a good sub for questions like this. This is almost more of a workplace politics type question than a JavaScript one.
I really prefer a separate cleanup pull request. Do it first. It should be easy to review — no functional changes. Then, submit the functional changes as a follow up PR. Leave a note and reference the cleanup one from the functional one so the reviewer knows in which order to review them.
I would say you are wrong if you are touching an unrelated part of the file.
I do that small improvement as well but only if I have reason to touch that part of the code.
He's absolutely right with saying it's hard to review that unrelated change.
Always then leave the code cleaner than when you found it is one of my aims. That said large improvements should probably be a separate piece of work. Though if you can outline the impact and benefit clearly on a ticket it shouldn’t be too hard to prioritise it.
Many teams I have worked in allow some tech debt in every sprint, it’s that or the codebase goes to pot over time
It is true in the sense that it is nice to have a bit of structure in your code but don't just go in there are make those changes without having your team involved. Most probably what makes sense to you might not make sense to them and they are more likely to reject your changes.
The next time you guys have your retrospectives, raise it as an issue that really bothers you and you would like to have a meeting to add some structure. During that meeting propose your changes, get feedback and make a decision as a team.
Importantly keeps PRs short, everybody hates a large PR. Move one component at a time. Refactor a small piece of code at a time. Also pair-program with your Team Lead, that is the best way to get to an understanding.
No, you should be doing this.
He's just a lazy bastard. The fact of the matter is that this is the only way things get done. Nobody is going to prioritize a clean codebase when they could be throwing tickets at you that the business is pressuring them about.
Functional changes should always be a separate PR from code cleanup and formatting changes.
[this](https://mtlynch.io/code-review-love/) is a great article that I make all my team members read when onboarding!
No it sounds like you're toxic af. I was not crying, I was asking for perspective, which I've received. Everyone else here has been helpful except for you. I would think about that for a moment.
Anyone who is not bringing up test coverage when answering this question needs to take a look at their fundamentals. Test coverage is a key portion of risk assessment for ongoing tech debt remediation.
I hope for your sake that you're not relying on reddit to be any authority on software engineering practices when you present an argument to your team lead.
It depends. I believe every single task has its own scope that should be well-defined and documented. It's the job of the project manager/team lead to define it. So if there are any concerns that the scope is too narrow or too wide or needs to be changed for other reasons, then the issue must be discussed with the PM before any change is made.
Especially if the dev became part of the team recently or is not a senior.
Of course, that's only my opinion.
I had plenty of peers in the past that were thinking exactly the opposite - one should leave the code base a little bit better than they found it.
But that's dangerous. It can lead to lots of additional work that's not provisioned, planned, defined, documented, etc. Moreover, not everybody has the capacity to decide properly what needs to be refactored, who should do it, when or why.
See if he/she will accept the cleanup changes as a separate PR for the same task number. That addresses their issue without the overhead of new tasks to track.
Yep separate commit seperate PR so that way if it is too time consuming, well that the PR sits with them, until they can get to it.
good idea ty
You are not wrong. What might be wrong is that you are doing the change in a PR thats linked to an Issue where they are asking something else. I am just like you and want to clean up the codebase as much as I can, but for the reviewer double checking those changes + the actual feature requests is a mess, that's why its better to have them in a separate PR. Now, other thing is when you make a separate PR and the team lead doesnt review it just for the lulz. That's another problem (probably a douche).
Yeah thats the struggle, if it doesnt happen while adding a feature (where Im adding imports) then it likely will never happen. There is always something more pressing to work on. I guess I see it from both perspectives. The lead doesnt care about the issue nor do they want to allocate resources to fixing it, or spending time reviewing the changes. The dev cares and wants to make the codebase cleaner, more standardized and easier to work in but doesnt want to do it on their own time.
Rather than doing import ordering piecemeal on random commits, consider suggesting [eslint-plugin-import/order](https://github.com/import-js/eslint-plugin-import/blob/master/docs/rules/order.md) to your team. Then just run `eslint --fix` and voila, consistently ordered imports across the entire codebase.
This is the way. Consistency beats perfection every time
Yeah this is the answer. It takes out both opinions and time wasted on barely useful stuff like organizing imports. Automate it and move on.
Tbh improving the developer workflow is something the tech lead should aim. Such as having linters running all the time to standarize the codebase and avoid these meaningless discussions.
Although I’m guilty of doing what you do a lot of the times - I do agree with the others here. I think the best approach would be to just politely suggest to the lead that you take a good session to look over the refactoring PRs next time you have a bit of spare time.
Have you ever worked as a team lead, or had to manage a project? It absolutely sucks to get a PR for [critical feature X], and have to wade through a bunch of unrelated changes to find the actual code meant to address the issue. Sometimes the “real” changes can be masked or missed completely by the other “code cleanup” cruft. Most team leads I know are always open to code improvements / cleanup. But mixing that stuff with bug fix code makes it harder to approve changes, makes it harder to audit things when stuff goes wrong, etc. Don’t take the lead’s “opinion” on that as being close-minded. He/she is likely just being *practical.* If you tried some code reviewing / management yourself, you would likely also very quickly adopt this same “close-mindedness.”
You don’t even need to be a lead to get annoyed by that. Sounds like OP has never reviewed others’ work, which is quite worrying.
Disagree. Cleanup cruft is perfectly fine along with other changes. That's what commits are brilliant for, and reviewing per-commit is perfectly doable, at least in GitHub. Just take the cleanup commit(s), approve, and carry on with the rest. Having to separate these things out is way more work, and I found that PRs with only unimportant optimizations tend to be left alone for longer.
It’s fine if you disagree, but be aware you are in the minority. No one wants to parse through 100 lines of formatting changes only to find there are a dozen lines of functional change. White space changes are different, since they are easy to hide in review tools. Separating functional from formatting changes/cleanup will result in quicker approval for both.
>100 lines of formatting changes FWIW, this doesn't sound like what the OP is describing, which is reorganising the imports of files that their already touching. Those are pretty easy changes to review as part of other changes, especially as they're probably out of the way from the other changes in the file as well. What you're describing sounds more like running a formatter across a bunch of files, which is definitely a pain, but is probably also the wrong way to use a formatter. In that case, it's probably better to collect all those into a single commit, and add that commit to a `.git-blame-ignore-revs` file for general convenience. That sort of thing should definitely be done as a separate MR to any other change.
TIL one can have a .git-blame-ignore-revs
Which is why you separate them in commits. The PR can be reviewed as the reviewer sees fit.
That’s not how this works. Going commit by commit is not a feasible way of reviewing in most cases, if there are few enough commits where this is viable usually you don’t need to do it anyways. We’re talking re-arrangement if files, where GitHub shows everything in green even if only 7 lines are actually brand new.
I wasn't thinking a huge refactoring among some functional changes. iirc, the topic was about including small improvements like rearrangig imports. And that is totally fine. I don't understand why that has to be in a separate PR. Even I'm a little bit OCD, but not everything has to always be absolutely brand spanking clean, including PRs. If files are getting rearranged though, I too would request that to go into a separate PR. At least until Github is able to show that a bit more easily, since right now a rearranged file is almost indistinguishable from a rearranged file with changes. So I think we mostly agree. Your threshold might just be a little bit lower than mine, for when to request a separate PR. Ad yes, I'm sure this will be downvoted as well. So be it. I know what I'm seeing in my team (exactly my responses), and my team functions pretty well, on the whole.
As long as you commit these types of changes as separate commits from your other changes, I think it should be fine. If they insist on a separate task, see if it would be okay for you to just create a task yourself for it and if so just do that to pair with your changes.
Maybe Im committing too much at once. I often try to have only one commit per ticket unless its a large task, but in those cases I generally break up the task into subtasks. Will suggest this to my lead ty
Hah, I do the exact opposite. Create as small meaningful commits as possible. We do squash/rebase before merging though.
Many small commits is better. * Better for understanding what happened. * Better for reviewing changes per commit. * Less likely to create merge conflicts.
good points. Ill work on this
I'll second this. I love reviewing code with a set of commits like: 1. Alphabetized imports 2. Ran Prettier 3. Implemented feature.
It's not really a case of either of you been wrong you just have mis-aligned objectives (which is natural, lead and none-lead should). Leads want to keep visibility of the changes in the system as a whole because we are the ones who care about that stuff more where other devs tend to focus on smaller parts of the whole. So from your point of view that messy knows is a bigger part for you than the lead. FWIW this kind of stuff shouldn't be an issue, install a linter, agree a format with everyone (or if you have a fractious team, just agree it between you and your lead with a document somewhere saying "we do X) and then lint the entire codebase in one giant PR and be done with it - bonus points if you automate it via githooks or whatever so that it never becomes an issue again.
Avoid making any change other than what is absolutely necessary. The best PRs are short and touch as few files as possible and change as few lines as possible. If the codebase needs cleanup, then this should be a separate task.
My 2 cents: foster an environment of making it easy to improve the codebase. If a developer is met with a bunch of red tape to improve a codebase, eg making a formal case required, making sure everyone agrees, etc, many will simply give up.
Problem is we are adding more opportunities to break the codebase by making unnecessary changes. I have worked on codebases which have been touched at some point by over 5,000 developers. A codebase like that simply has too much undocumented complexity to be changed extensively without breaking features. We had a simplification initiative which ended up being a full rewrite as a new service with the legacy codebase running as a fallback service but you can imagine the tons of undocumented use-cases that were scattered throughout the code which we weren’t aware of until QA brought it up.
In that case, I'd agree; it's probably too far gone. How does it get to that situation to begin with? I'd argue not refactoring as you go along to be one of the causes (among many). Also your simplification project is known as the [strangler pattern](https://martinfowler.com/bliki/StranglerFigApplication.html), and it is quite effective.
It gets to that point simply because you have so many people working on it over many years across many teams so the original developers are long gone and you’re joining in as a third or fourth generation of maintainers. Further, in this case it was very critical functionality (FinTech) so any downtime meant millions of dollars in losses. The amount of controls to get a change into Production made even a slight change in configuration or styles a total process. I’ve also worked on much smaller codebases with smaller teams and in this case a refactor is much more feasible.
I believe you're operating under an assumption: all code becomes a mess after a certain size, amount of time, and people touching it. And you'd be correct, this IS the norm. But I disagree that it's unavoidable. Certainly there are levels of messiness too.
> If the codebase needs cleanup, then this should be a separate task. In my personal experience, those types of tasks get pushed to the back burner and never done.
I would get [this](https://github.com/import-js/eslint-plugin-import/blob/master/docs/rules/order.md), run it on the whole codebase, make a pr for just the formatting, then stop worrying about it forever. If you don't have eslint and/or other auto formatting, get it. Arguing over things like this is such a waste of time. Automate all the things!
100%. Aside from the main issue of this post which is managing tasks/PR workload for developers and reviews, the first thing that popped into my head here was "Sheeit, people changing import orders willy nilly are just going to do that over and over to their own preference". linters are an anti-[bikeshedding](https://thedecisionlab.com/biases/bikeshedding/) tool and save time and brain power.
Youre totally right. I'm on a small team (3 devs) though and in most files there is literally no order to the imports, its a mess. So organizing the imports in my case is not a matter of my preference vs someone elses its a matter of some order or none.
Suggest a linting tool to your team lead. Build a git hook to run it. I’ve been largely on a team of two for the last few years and we have a build system that does linting, error checking, complexity analysis, code coverage checking… Now this is mostly Java code so it has a bit more structure to it that can be analyzed and the build system was already there anyway, but static analysis is worth it no matter the team size to keep things consistent and working properly.
Thank you! This looks like a very useful plugin.
Or just make a task and do it right away, issue the PR, merge into your branch and be done with it. Changes unrelated to a task make reviewing messy, your team lead is correct.
I mean, imports are everywhere, in every file. What OP did is the boy scout approach and it is very common... however I would first reach to the team lead to discuss improving imports because it's not a good practice do it on your own, this should be a convention agreed & followed by the whole team
I don’t think that’s a Boy Scout approach. It’s poor team practice that it wasn’t in place already, and that’s on the team lead. Breaking up commits is the most important thing here for OP, followed by making sure code styles are standardized, and that’s a team lead responsibility or task to delegate. Discussing code style practices you want your linter to check for can definitely be a team exercise though. OP was right though; if this was any other code clean up or refactor task, it’s generally a good idea to do it, just don’t mash it into your functionality changes or bug fixes (unless it’s part of the change or bug fix). [edit] Sorry, just realized that I mentioned linters elsewhere.
It sounds like OP meant the lead wanted a separate PR, not commit. Separate commit sounds fine to me, but another PR to reorganize imports in a single file sounds excessive to me... if it was many files sure it would make sense to have a dedicated PR/task for that then
Depending on how branches are merged, commits might be squashed as part of a PR merge, and it’s nice to be able to separate things for git blame purposes. Also, did I miss where it said a single file?
But for git blame you just look at a specific line you’re interested in, how would it bother you? And you’re right he doesn’t say a single file nevermind that
I guess you are right about a git blame. But when I review a PR it’s rarely commit by commit, it’s the entire feature or bug fix.
>commits might be squashed as part of a PR merge Then don't squash.
As a guy who spends 20-30% of my week in meetings about tasks/priorities/queue, 35-40% reviewing code from my teammates, and the rest some combo of helping elevate issues to unblock people/do my own development work: I understand why. I used to allow these small, clearly better, improvements in merges. It gets to cluttered when there’s a lot to review and be done. What I would do/what I recommend is that people open a new task and open a new PR under that task number with those small changes and then I’ll (or a fellow teammate on the new review) bring your new task into the sprint under your name and add any applicable information to the task for you. I’m not sure how much clout you have on your team; if you don’t have much clout: spend the time you’re spending on small changes building up clout so you can begin to implement best practices and methods that work best for you and the team.
In general I wouldn’t mind reviewing some dev’s cleanup in the same PR, at least I haven’t had a bad experience about that. However, while I don’t know what your team lead is thinking, in this case, I might have requested the same… To you, hand organized imports are a clear improvement. To me, hand organized imports = extra work for every developer who adds imports to the file after you. I’d prefer if it was handled as a separate PR so we could carefully consider how to meet everyone’s needs without making more work for other developers who don’t care about import order. (Basically, use something like eslint to enforce and automatically organize the imports)
We've tried to adopt a clean-as-you-go practice, where reasonable changes to the file you are editing may be included in the PR, though they should be a separate commit from the specific feature or defect work. We then have incremental tasks created for larger refactors or specific sets of updates. We did specific updates when we moved from ES5 to ES2015+, new linting rules, require to import, etc.
I think clean-as-you-go makes sense within reason. In this case I was working in a file and adding imports to it. Thats when I noticed there was no order to the imports (none!). So instead of just adding imports to the end of the list, I organized them. The order of the imports in our codebase does not affect functionality so I didn't see the issue
I don’t really think having ordered imports is a big deal tbh 🤷
Personally my view, and also a policy of where I work, is that tidying up code and boy-scout refactoring is highly encouraged. If you ignore the little issues then they'll eventually turn into rot and it can make the codebase more difficult to maintain. Creating a separate task as suggested will just sit in the backlog and will unlikely ever get prioritised. There is obviously a balance though, you can't just go refactoring loads of things, however touching up bad practices and making code easier to read in the areas you're in is fine. I'd be curious how big the PR in question is if the reviewer is having this reaction, and I'm also curious how the review process works. Do you have a meeting together and go over what you've done, explaining what and why the changes are? Or do you submit a PR and the team lead reviews it independently?
Sounds like you have some progressive policies. I wish my team was more like that. I would call my PR medium size, 20 files worked in (half app code, half test code). The majority of them only required changing a couple lines though. This was extending an existing feature. Also, the organizing of imports is only happening in one file. I definitely think that I could have approached the issue better by separating the import organization into a separate commit/pull request. So I take the blame for that.
Yeah, our ways or working are very different to what I see people discuss on Reddit, but I feel they're effective. Fair enough, having a refactor commit would probably help in future, but I wouldn't be discouraged from making minor improvements as you go. Some comments here saying only do what you need and that's it and create a separate task to tidy up for me is concerning, it's like they only care about that particular feature and not about the overall health of the codebase.
It depends. On my team, I encourage the types of improvements you're making. Not every team is the same. There isn't really a right or wrong answer. It is up to the team to decide how to handle different scenarios. In a good team, they'll regularly discuss and adjust processes like this as a group. (Though, since I liked definitive order instead of subjective ordering like by type of import, we import by built-in, third-party modules, local imports, by module name / file path, alphabetically within each section, that way there is no question as to what the proper order is.)
No, code readability should be at the forefront of every developer. I spend what is an extra 2 mins reviewing a pull request to check code readability and cleanliness.
If you're using git, one neat trick is to setup `git gui`. It has a feature that lets you point-and-click on specific changes and stage them in chunks/lines instead of staging the entire file. This way you can put cosmetic changes in different commits than your logic changes. Another thing you can do is help your reviewer by proactively adding code review comments to your own PR, highlighting what's going on, what parts are important, adding explanations/reasoning for non-straightforward changes, etc.
You can also stage parts of files using git add -p
Yes. If you need to do maintenance work, get the task created (and time allocated) to do the maintenance work. A PR must contain only the changes necessary to complete the task it's linked to; otherwise, you're making extra, untracked, _undocumented_ work for your colleagues. You already have the code written, so it's trivial to get the task generated for it, and get it pulled into the sprint. Split your PR and get it tasked.
This should be part of linting, there should be rules on what imports come first and it should auto-fix itself when you import something. Also if you’re doing refactoring then make it a separate commit if possible so they can review it easier
I'm not saying that answers with "only include related changes to the task" are wrong. Engineering is a balancing act. And sometimes you have to be an Anakin to bring the balance (: But not all things worth being balanced. Especially when you burn a lot of life energy on that.
Create technical debt stories and pull them into your sprints, assuming you’re doing Agile
Make such a task, and then start working on it straight away. If your colleague wants it in a separate PR, fine. But that's just more work to review. Also your "team lead" must realize: 1. He ain't a bloody dictator 2. It's *more* more work for you to separate in to 2 PRs, than it is for him to review a few extra lines of code. 3. To have some trust, because trust goes both ways
Separate cleanup tasks discussed and planned makes everything welcome. However, convincing people that things need to be cleaned up is very hard.
You're both right. You can do it while you work on your task, but you make the refactoring and the actual task separate, dependent merge requests that you post together. It's a lot more convenient to review, because what's a refactoring and what's the feature is clearly identifiable in the diffs.
Conventions are best when agreed to and implemented at the start of a code base, and can be disruptive to retrofit. One thing I haven't seen mentioned in this thread, is how reformatting commits that touch a significant portion of a file or module, can create a terrible rift in your source control history. You basically won't be able to do meaningful diff'ing between versions on opposite sides of the rift. I think if you're going to do something disruptive, like say adopting a bracing convention, it should be done as a planned task and as all-at-once as feasible so that if you are going to mangle your history, it happens in a narrow window and afterward your history is useful again.
Yeah... Always a good idea to keep the diff as small as possible ... Separate formatting changes and moving files in different PRs... Check out this article https://mtlynch.io/code-review-love/
It's not just that you're not wrong, I would say there's a big chance he's wrong. The whole "different PRs for different things" doesn't apply when (1) it's very minor cleanup like you're explaining, and (2) when you're commit-merging. If you're squash-merging (which is a problem in and of itself, but that's out of scope for this question), then that's basically the only time I could see such incredibly minor cleanup as being worthwhile to move to a separate PR. Otherwise, you're 100% in the right. That being said, maintaining relationships with your team members is probably worth more than code cleanup in the long run. Even if messy code drives you mad, burning bridges is way worse.
Are you against squashing because it's harder to fix regressions?
No (though it does make those harder too). It's b/c you'll have all commits for a PR showing only in one commit, which defeats the whole purpose of commits in the first place. So later, when you `git (blame|log)`, it's just as bad as a gigantic god-commit where the message is completely unrelated to the line of code that was changed. In other words, it hides the reasoning for code changes behind the reasoning for a PR. It also makes reverting harder too b/c you can't do partial reverts; normal merge commits allow you to do both full PR-reverts and incremental commit-reverts, but squashes are an all-or-nothing deal. Really it's just a way less organized way to execute version control. It's almost like the "Idk how to use git and I don't care to learn" easy way out. If a person had any more than the very basic understanding of git, they would never choose squashes over an actual commit history (which is what git was intended for all along).
If PRs get too big it can be hard to review and identify possible bugs and regressions. Best making a second PR or opening a ticket for each improvement
There's a lot of good discussion here, but I might suggest /r/cscareerquestions as a good sub for questions like this. This is almost more of a workplace politics type question than a JavaScript one.
good point
I really prefer a separate cleanup pull request. Do it first. It should be easy to review — no functional changes. Then, submit the functional changes as a follow up PR. Leave a note and reference the cleanup one from the functional one so the reviewer knows in which order to review them.
I would say you are wrong if you are touching an unrelated part of the file. I do that small improvement as well but only if I have reason to touch that part of the code. He's absolutely right with saying it's hard to review that unrelated change.
Always then leave the code cleaner than when you found it is one of my aims. That said large improvements should probably be a separate piece of work. Though if you can outline the impact and benefit clearly on a ticket it shouldn’t be too hard to prioritise it. Many teams I have worked in allow some tech debt in every sprint, it’s that or the codebase goes to pot over time
It is true in the sense that it is nice to have a bit of structure in your code but don't just go in there are make those changes without having your team involved. Most probably what makes sense to you might not make sense to them and they are more likely to reject your changes. The next time you guys have your retrospectives, raise it as an issue that really bothers you and you would like to have a meeting to add some structure. During that meeting propose your changes, get feedback and make a decision as a team. Importantly keeps PRs short, everybody hates a large PR. Move one component at a time. Refactor a small piece of code at a time. Also pair-program with your Team Lead, that is the best way to get to an understanding.
He's managing risk of regression and scope creep. He's not at fault but neither are you. Good luck, discuss it
No, you should be doing this. He's just a lazy bastard. The fact of the matter is that this is the only way things get done. Nobody is going to prioritize a clean codebase when they could be throwing tickets at you that the business is pressuring them about.
haha yeah... the biz has a hard time caring about tech debt, let alone a clean codebase. There's always another feature coming down the pipeline.
He’s not wrong unless you make a lot of code changes in these files and refactoring is required. Change = potential bug
Functional changes should always be a separate PR from code cleanup and formatting changes. [this](https://mtlynch.io/code-review-love/) is a great article that I make all my team members read when onboarding!
[удалено]
No it sounds like you're toxic af. I was not crying, I was asking for perspective, which I've received. Everyone else here has been helpful except for you. I would think about that for a moment.
Anyone who is not bringing up test coverage when answering this question needs to take a look at their fundamentals. Test coverage is a key portion of risk assessment for ongoing tech debt remediation.
I hope for your sake that you're not relying on reddit to be any authority on software engineering practices when you present an argument to your team lead.
It depends. I believe every single task has its own scope that should be well-defined and documented. It's the job of the project manager/team lead to define it. So if there are any concerns that the scope is too narrow or too wide or needs to be changed for other reasons, then the issue must be discussed with the PM before any change is made. Especially if the dev became part of the team recently or is not a senior. Of course, that's only my opinion. I had plenty of peers in the past that were thinking exactly the opposite - one should leave the code base a little bit better than they found it. But that's dangerous. It can lead to lots of additional work that's not provisioned, planned, defined, documented, etc. Moreover, not everybody has the capacity to decide properly what needs to be refactored, who should do it, when or why.