|
MadFriarAvelyn posted:I had one reviewer have a very ageist stance during a pull request today and I'm debating whether or not to shrug it off or drop it on HR to deal with. You could always talk to the person, especially if you're in a position of seniority. Let them know that it's not an attitude that's acceptable to the rest of the team, or even just to yourself if that's not the case. Doesn't matter if you don't have firing authority, losing the respect of your peers is a legitimate threat. But it sounds like it bothers you, so ignoring it doesn't seem like a good option, and if it's the first time it's happened taking the nuclear option (HR) might be a bit much. You can always escalate if it happens again.
|
#
?
May 29, 2021 23:31
|
|
|
|
| # ? May 9, 2024 05:56 |
|
|
Ghost of Reagan Past posted:On that note can I just say how liberating it is to select a code style and enforce it from the get-go so there's no debate? My boss (the CTO) asked if I'd used it at previous jobs. No, no, I just don't want to debate code style. Black is good enough, it enforces a style, that's literally all I care about : ending debate about about style and forcing everyone to write code in a particular way. We also find this to be very useful. So many long and tedious discussions just immediately disappear.
|
|
#
?
May 30, 2021 14:31
|
|
|
Working go fmt into the standard dev workflow is phenomenal for this.
|
|
#
?
May 30, 2021 15:47
|
|
|
How do people work linters into their team’s flow? Ideally (to me) everyone would just use a real editor that could reliably run on-save hooks, but apparently this is too much to ask... Simply marking a PR as failing unless the code matches linted output feels a little petty, but actually committing linted code and pushing from CI feels a little inflexible.
|
|
#
?
May 30, 2021 16:27
|
|
|
good jovi posted:How do people work linters into their team’s flow? Ideally (to me) everyone would just use a real editor that could reliably run on-save hooks, but apparently this is too much to ask... Simply marking a PR as failing unless the code matches linted output feels a little petty, but actually committing linted code and pushing from CI feels a little inflexible. pre-commit it hooks into git and runs the script before committing. If it changes the files it doesn't actually commit.
|
|
#
?
May 30, 2021 16:46
|
|
|
good jovi posted:How do people work linters into their team’s flow? Ideally (to me) everyone would just use a real editor that could reliably run on-save hooks, but apparently this is too much to ask... Simply marking a PR as failing unless the code matches linted output feels a little petty, but actually committing linted code and pushing from CI feels a little inflexible. We do exactly what you're describing. The linter runs as the very first step of the CI build, and by "runs" I mean it inspects the code to see if it is formatted correctly or not, but doesn't actually proactively do any reformatting. If inspection fails, the build fails. That happens very quickly, like within a few seconds. I agree that it does seem petty to completely fail the build because someone put an extra line break between two functions, but as mentioned above we get real gains from that standardization of formatting. (My personal favorite: I can grep confidently for particular code constructs because I know exactly how the whitespace has to be placed across all of our code.) Beyond that, it's on individual developers to format their code correctly in order to pass CI. They can run the linter step locally, run it with the --fix flag, add save hooks to their editor, learn the rules and format the code by hand, whatever they want. We don't have an automated step during CI which fixes and commits the linted code. Possibly we could save ourselves a little effort by doing this, but there are quite a few linting rules which can't be automatically fixed in this way, so it doesn't help in the general case.
|
|
#
?
May 30, 2021 16:48
|
|
|
good jovi posted:How do people work linters into their team’s flow? Ideally (to me) everyone would just use a real editor that could reliably run on-save hooks, but apparently this is too much to ask... Simply marking a PR as failing unless the code matches linted output feels a little petty, but actually committing linted code and pushing from CI feels a little inflexible. We use formatters with configurations that can be read by our IDEs with format on save. There's a build command you can run locally that does the same thing as our build pipeline, to run all the tests and whatnot. If you forget to run the code format step the pipeline will run it and commit the changes for you. Worst case you just have to do a pull afterward.
|
|
#
?
May 30, 2021 17:53
|
|
|
We used containers for development but the real world performance of docker on MacOS was so bad we ended up going back to figuring out how to just run everything locally on our machines. Our one developer was on a slightly order MBP (like 2015) and every request to his local containerized web server was taking 4+ seconds.
|
|
#
?
May 30, 2021 17:59
|
|
|
I combine the git pre-commit hook and the CI step. If the linter fails, the CI job fails. The project readme includes a pre-commit hook script so that it's easy to avoid this.
|
|
#
?
May 30, 2021 18:23
|
|
|
If you say that you require a particular coding style but you don't fail the builds on lint errors, then you don't actually require it.
|
|
#
?
May 30, 2021 19:05
|
|
|
Also, having both pre-commit and CI checks reveals how many people skip the hooks.
|
|
#
?
May 30, 2021 19:47
|
|
|
Just bear in mind you'll want to have an escape hatch of some kind, in case of linter bugs or legitimate, articulable reasons to not follow the guide in a particular place.
|
|
#
?
May 31, 2021 00:33
|
|
|
Pre-commit hooks are dumb because they negate one of the great things about git. Immediately before running a tool that's going to gently caress with your source code is exactly when you want to make a commit so that you can undo it in the off chance that something goes wrong, and then if it doesn't go wrong you can just amend the commit. pre-push commits are a bit more sensible, but there's still plenty of times you'll want to push some WIP stuff.
|
|
#
?
May 31, 2021 00:41
|
|
|
Plorkyeran posted:Pre-commit hooks are dumb because they negate one of the great things about git. Immediately before running a tool that's going to gently caress with your source code is exactly when you want to make a commit so that you can undo it in the off chance that something goes wrong, and then if it doesn't go wrong you can just amend the commit. pre-push commits are a bit more sensible, but there's still plenty of times you'll want to push some WIP stuff. That's why you have --no-verify
|
|
#
?
May 31, 2021 01:03
|
|
|
Volmarias posted:Just bear in mind you'll want to have an escape hatch of some kind, in case of linter bugs or legitimate, articulable reasons to not follow the guide in a particular place. Ideally this is by some marker in the source code (such as a // NOLINT comment or similar), so that - it doesn't end up failing for the next person to edit that bit of code - if the same exception happens a lot, you're motivated to fix the linter so that you can get rid of those ugly comments
|
|
#
?
May 31, 2021 03:28
|
|
|
Alright, now I'm getting frustrated. New PR, I refactored my work multiple times before submitting. I cleaned up the edges. I paid closer attention to structure and function location. I did more testing work to catch edge cases. I improved my comments to explain why the code existed the way it does. I reviewed my code as critically as I could and made tough edits. Once again, absolutely obliterated in code review. This time with way more "you" statements that make it seem much more personal. Did I make some mistakes across my couple hundred lines of code? Yeah, I'll own those, it was a hard PR. But to slap a "you're not really close here" in the comments on initial review is pretty painful. Especially when I worked my rear end off to get the loving thing to actually work in the first place.
|
|
#
?
Jun 4, 2021 18:30
|
|
|
About time to have a chat about expectations and feedback?
|
|
#
?
Jun 4, 2021 18:52
|
|
|
Judge Schnoopy posted:Alright, now I'm getting frustrated. What I've found is that when people do something like this, they disagree with your approach on the high level. Unfortunately they might have what this "high level" approach is in their heads and not actually articulate what it is (there might even be a design doc somewhere they're assuming you have read), and then go through and mark the PR up using your code as examples of "hey, you're doing a bunch of stuff you shouldn't have to do or won't work in all cases because the PR didn't adhere to the high level design." So I guess the steps here would be: a) Do I have a high level design for this PR (100's of lines of code implies a new feature to me)? b) Did this person go over the high level design? If your reviewer disagrees on the high level design, it doesn't really matter how much you've tested etc, they're just going to keep tearing the PR up. That's pretty lovely on the reviewer's part, but sometimes it's worth asking how they would implement the feature if it were up to them and see if they have a response.
|
|
#
?
Jun 4, 2021 18:58
|
|
|
Yeah, it's not going to be a comfortable conversation but it sounds like it is necessary.
|
|
#
?
Jun 4, 2021 18:59
|
|
|
Re that comment: Not really close to what? Correct functionality, or the exact design the reviewer has in their head?
|
|
#
?
Jun 4, 2021 19:04
|
|
|
Even as someone who can take a beating in critiques/reviews thanks to design school, I can say that that code review process feels problematic. Two main issues I see: - Lack of earlier communication in regards to what the reviewer/team lead is expecting from you. Was there any sort of planning that happened in which you talked about how you were intending to execute this feature? Were you given specs detailing high level implementation or just bare bones input/output requirements with no structural guidance? If they didn't specify how they wanted you to go about designing/structuring the thing, then that's on them. Did your reviewer communicate with you about your progress throughout the sprint and make sure you were moving in the right direction? They should have done that, especially for a new person. - Being dickishly coy. This is also a communication problem but distinct from the aforementioned lack of pre-review communication. A code review isn't a loving mystery novel where you keep things hidden from the reader until just the right moment for maximum suspense. If there's a problem, the reviewer needs to not only say so, but also explain, in plain terms, why it's a problem and how they want it fixed. "You're not really close here," is not only utterly useless and unhelpful, but below-the-belt mean. Appropriate response, in my book, would be more like, "Your solution to X is incorrect/suboptimal/falls short because it fails to account for scenarios A and B. To properly handle all scenarios, you need to change Y and add Z" or whatever. No mystery, no dickishness, only information you need to correct the code at hand. Explaining what the issue is will help you learn and help them get the result they want. Making you try to read their minds will not help you learn and not help them get the result they want.
|
|
#
?
Jun 4, 2021 21:23
|
|
|
Feedback that is not concrete is garbage. Even if they want a whole new approach they should be asking for you to do a design doc or other write up that you can hash it out over rather than waste your time going over and over the same code while playing "guess my intent".
|
|
#
?
Jun 4, 2021 21:23
|
|
|
Judge Schnoopy posted:Alright, now I'm getting frustrated. Yeah this means schedule a one-on-one because communication is hard. As a bonus your reviewer will probably find it harder to be aggressive.
|
|
#
?
Jun 4, 2021 21:24
|
|
|
At the very least, something like "You're not really close here" needs to be followed up with very specific critique. Is it in violation of style guides? Is the run time bad? (if so, how?) Is there actually just a set of bugs at run time here that they spotted? Furthermore, you don't need to actually pepper that kind of feedback with that attack. You can just say, 'The runtime here is super bad. You shouldn't be searching a list n times for a result, you should be using a hashmap which saves substantial time' (or whatever the problem would be). +1 to schedule a one-on-one
|
|
#
?
Jun 4, 2021 21:40
|
|
|
Brain Candy posted:Yeah this means schedule a one-on-one because communication is hard. As a bonus your reviewer will probably find it harder to be aggressive. However, they may not wish to meet, in which case they're finding it easier to be passive.
|
|
#
?
Jun 4, 2021 21:41
|
|
|
The "not really close" comment was an addendum to a shopping list of specific critiques in the code. More of a summary of the thoughts. And it still stings a bit. I got another review from a senior who added a few good points but said overall it looked ok. This is the senior that I spoke to about high level design, which the other senior called into question during the review. The design was not written out, the user story was severely lacking details, and the scope wasn't accurate, all leading down this trail of disappointment. I would say it's a lesson learned but I didn't know any of the code around this feature when I started, and expected the ones handing me the work to have a grasp of what that work entailed. If I could go back in time I still wouldn't have been able to fill out the design / story / scope. I talked it over with my manager today, who said he's been watching this PR (albeit silently). He thinks I'm doing a good for just starting out. I reassured him that I know there is a lot of learning and improving ahead of me and I'm not asking for review to be a pillow fight. Overall feeling better in general but still kind of salty. I'm definitely going to get him on the phone for the re review so I can talk through his comments and immediately address new feedback.
|
|
#
?
Jun 4, 2021 22:49
|
|
|
Judge Schnoopy posted:The "not really close" comment was an addendum to a shopping list of specific critiques in the code. More of a summary of the thoughts. And it still stings a bit. Ask your friendlier reviewer to address the comments that the less friendly reviewer has made, see if you can get them to argue in your comments. Let them fight.
|
|
#
?
Jun 5, 2021 00:11
|
|
|
Volmarias posted:Ask your friendlier reviewer to address the comments that the less friendly reviewer has made, see if you can get them to argue in your comments. Let them fight. Is this also how to deal with good cop/bad cop during an interrogation?
|
|
#
?
Jun 5, 2021 07:18
|
|
|
Judge Schnoopy posted:Alright, now I'm getting frustrated.
|
|
#
?
Jun 5, 2021 08:18
|
|
|
Oh, you have two warring seniors and a manager who apparently doesn’t want to openly get between them. This should be a fun ride 
|
|
#
?
Jun 5, 2021 14:31
|
|
|
Prism Mirror Lens posted:Oh, you have two warring seniors and a manager who apparently doesn’t want to openly get between them. This should be a fun ride Nah, sounded like one who is chill and one who is unchill, and a manager that doesn't care about code reviews that much.
|
|
#
?
Jun 5, 2021 14:44
|
|
|
Brain Candy posted:Nah, sounded like one who is chill and one who is unchill, and a manager that doesn't care about code reviews that much. "How's this going to affect our burndown chart, again?"
|
|
#
?
Jun 5, 2021 15:07
|
|
|
marumaru posted:"How's this going to affect our burndown chart, again?" And by 2023 we’ll employ everyone on Earth. … And we don’t have the parking for that.
|
|
#
?
Jun 5, 2021 15:19
|
|
|
Brain Candy posted:Nah, sounded like one who is chill and one who is unchill, and a manager that doesn't care about code reviews that much. It's this except the manager is trying to mentor the unchill senior into being a force multiplier, which he still has a long way to go. I looked back over the last 4 months of review at this company. Nobody got half the comments or attitude as I get from unchill senior. Either I'm the worst, which manager doesn't seem to think so, or this guy just really doesn't like me.
|
|
#
?
Jun 5, 2021 15:30
|
|
|
There's maybe a small chance that Unchill thinks they're being Socratic
|
|
#
?
Jun 5, 2021 16:02
|
|
|
Judge Schnoopy posted:It's this except the manager is trying to mentor the unchill senior into being a force multiplier, which he still has a long way to go. it's entirely possible unchill senior is just poo poo lots of people get to senior not by being good at their job but instead by being an obstacle to progress in the sense that 'we need this person to make progress'. often it's because of their own lovely behavior and actions like gatekeeping others from being meaningful contributors do you have any kind of mentor relationships you can fall back on here? share the code and the review comments with someone with your interests in mind and not the companies (like your lovely manager who is probably terrified to piss off unchill senior because they're seen as critical to business continuity) and see what they think. if you don't have that kind of relationship with anyone feel free to pm me and i can take a look
|
|
#
?
Jun 5, 2021 17:33
|
|
|
Nah I mean I totally made some technical mistakes and am owning those. Both chill and unchill have been with the company from the very start, they built the product from the ground up. Unchill seems the type to want ocd levels of control over making all the code up to his quality. He's an extremely talented engineer and his reviews have this insane understanding of exactly how the new code interacts with every bit of the software. My only problem is the approach of "you're not meeting the quality standard, do better" instead of "hey let me point you in the right direction and give you some exercises to improve your weaknesses"
|
|
#
?
Jun 5, 2021 17:45
|
|
|
Many 'seniors' are senior at individual contribution but terrible at communication,. esp. if freshly senior. Find out whether this person is truly an rear end in a top hat or only bad at expression with you. Sometimes the frustration that bleeds through is the frustration of the person being unable to express themselves. Obv. this is not fun and is not something you have to roll over for, but the best reviews happen after a mutual agreement on style of expression and what's at stake. Talk to people! Talk!
|
|
#
?
Jun 5, 2021 17:53
|
|
|
Judge Schnoopy posted:He's an extremely talented engineer and his reviews have this insane understanding of exactly how the new code interacts with every bit of the software. Rocko Bonaparte fucked around with this message at 00:31 on Jun 6, 2021 |
|
#
?
Jun 5, 2021 17:53
|
|
|
|
| # ? May 9, 2024 05:56 |
|
|
I unilaterally decided that we're going to use Black to format my company's Python code and god, it's liberating to not care about style. Some of its choices are ones I wouldn't make but that's the beauty of it: I didn't make the choices. It just settles stylistic debates and we can move the gently caress on. Helps that we're doing this at the start of a project, of course, and that I have the authority to just decree such things, and we can just move on with our lives. Not a perfect choice for everyone but I'm a real fan of not having options in code formatting (it helps that outside an edge case or two it improves readability).
Ghost of Reagan Past fucked around with this message at 17:35 on Jun 12, 2021 |
|
#
?
Jun 12, 2021 17:27
|
|
