They can see the policy working...
-
Except those imports were used by a huge section of code you temporarily commented out, and now you'll need to manually select a dozen imports to get it working again when you come back to it.
(Sure you could have just commented out the unused imports, but the linter auto-sorted them and you're feeling too lazy to copy-paste a dozen scattered lines)
Use a good IDE, and readding the imports is pretty easy.
I find commented code to be a bit of a smell on its own, just delete it, and if you really need it again, dig it out of source control.
-
Code reviews?
"It passed CI, LGTM"
-
Use a good IDE, and readding the imports is pretty easy.
I find commented code to be a bit of a smell on its own, just delete it, and if you really need it again, dig it out of source control.
Yeah. My last job, a PR with commented out code typically wouldn't get approved. Either leave it in version history, or stick it on a branch
-
Code reviews?
wrote on last edited by [email protected]The developer I report to was in charge of that side of the code base and doing all the code reviews for it .... I thought he had a handle on it
-
That is what version control is for.
This. Commenting out code is bad practice
-
Except those imports were used by a huge section of code you temporarily commented out, and now you'll need to manually select a dozen imports to get it working again when you come back to it.
(Sure you could have just commented out the unused imports, but the linter auto-sorted them and you're feeling too lazy to copy-paste a dozen scattered lines)
Honestly I'll disable linting across entire files during these kinds of refactors because it's annoying having build output littered with unused imports and format warnings while I'm still working on a solution. Requires some extra diligence to re-enable and clean up before pushing though.
-
"It passed CI, LGTM"
My eye just twitched
-
Honestly I'll disable linting across entire files during these kinds of refactors because it's annoying having build output littered with unused imports and format warnings while I'm still working on a solution. Requires some extra diligence to re-enable and clean up before pushing though.
Use pre-commit with a linter config that checks all files, local dev can have a different config to disable as needed.
-
Except those imports were used by a huge section of code you temporarily commented out, and now you'll need to manually select a dozen imports to get it working again when you come back to it.
(Sure you could have just commented out the unused imports, but the linter auto-sorted them and you're feeling too lazy to copy-paste a dozen scattered lines)
During code review, we reject PR’s with commented out code. Problem solved.
-
This post did not contain any content.
Hot take: Clean up your darn imports. Otherwise you just make the links between modules confusing and messy.
-
This post did not contain any content.
The duality of Programmer Humor: You get posts complaining about bloat, and posts complaining about this.
-
My eye just twitched
Move over vibe coding, we vibe reviewing now
-
This. Commenting out code is bad practice
In general, I'm with you
But sometimes I need to revert/comment out a code block, because another code part isn't finished/working as it should.
Sure, it clutters code, but if I just comment out a function call and temporarily replace it with the workaround, it should imho stay in code.Else the workaround will stay forever and the commented out code will act as a reminder, that this part isn't clean yet.
But maybe it really is a case by case thing, where sometimes it's better to branch it out for later merge - although that can get really messy, while having the future implementation commented out, others will also see, how it is supposed to work and don't try to further extend the workaround, which makes future merging hell
Out of interest, how would your best practice look in such cases?
-
Hum... Ignore linter advice for code that you temporarily mangled.
It's not like you have to act upon it as soon as a blue line appears under your code.
Depending on the configuration, a linter may cause the compilation or a CI pipeline to fail.
-
The duality of Programmer Humor: You get posts complaining about bloat, and posts complaining about this.
Competent people vs incompetent people.
-
The duality of Programmer Humor: You get posts complaining about bloat, and posts complaining about this.
-
Depending on the configuration, a linter may cause the compilation or a CI pipeline to fail.
Failing your local compilation due to linter problems is just stupid.
Sending "temporary" changes into your CI pipeline isn't even stupid, it's borderline malicious.
-
In general, I'm with you
But sometimes I need to revert/comment out a code block, because another code part isn't finished/working as it should.
Sure, it clutters code, but if I just comment out a function call and temporarily replace it with the workaround, it should imho stay in code.Else the workaround will stay forever and the commented out code will act as a reminder, that this part isn't clean yet.
But maybe it really is a case by case thing, where sometimes it's better to branch it out for later merge - although that can get really messy, while having the future implementation commented out, others will also see, how it is supposed to work and don't try to further extend the workaround, which makes future merging hell
Out of interest, how would your best practice look in such cases?
wrote on last edited by [email protected]I would make it a TODO so that it's clearly temporary and so the linter bugs me about it until the intended permanent code is restored.
In general I prefer to keep separate branches and maybe a draft PR open for visibility for that kind of situation, though.
-
Hot take: Clean up your darn imports. Otherwise you just make the links between modules confusing and messy.
Sounds like a perfectly normal take.
-
Failing your local compilation due to linter problems is just stupid.
Sending "temporary" changes into your CI pipeline isn't even stupid, it's borderline malicious.
Sending “temporary” changes into your CI pipeline isn’t even stupid, it’s borderline malicious.
No? “Hey customer, I’ve deployed the changes you requested to the staging area. Is this what you had in mind? Keep in mind it only looks good and isn’t fully functional yet.”
