|
Vulture Culture posted:We culturally prefix nitpicks with "Nit:" in the comment text and interpret this as "I really don't care if you change this or not, but did want to bring this to your attention in case it was unintentional" Yeah, I'm a fan of this - I find it helps me at least flag when I'm getting into bikeshedding (because I will never stop bikeshedding) and otherwise let people know when what I'm proposing is less "this is objectively correct" and more "I like this better and you might too?"
|
#
?
Apr 13, 2021 02:38
|
|
|
|
| # ? May 9, 2024 18:29 |
|
|
Vulture Culture posted:We culturally prefix nitpicks with "Nit:" in the comment text and interpret this as "I really don't care if you change this or not, but did want to bring this to your attention in case it was unintentional" I strongly disagree with nitpicking and tagging them as nits. It just doesn't make sense, either it's an issue and should be fixed or it's not and there's no reason to bring it up as an issue. If you have a suggestion, then pick the instance, or one if there's multiple, and write the comment to clearly communicate that it's suggestion preferably with the review system not marking it as an issue if it allows it.
|
|
#
?
Apr 13, 2021 04:39
|
|
|
asur posted:I strongly disagree with nitpicking and tagging them as nits. It just doesn't make sense, either it's an issue and should be fixed or it's not and there's no reason to bring it up as an issue. If you have a suggestion, then pick the instance, or one if there's multiple, and write the comment to clearly communicate that it's suggestion preferably with the review system not marking it as an issue if it allows it. This is what it means in my organization to prefix a comment with “[nit].” We use Gerrit so we’re not exactly spoiled for metadata options, and as a result comments have to serve multiple roles for “this needs fixed,” “here’s a suggestion,” “oh I wish I’d thought of that,” etc. Our accepted internal practice is that if it’s “tagged” as a [nit] then it’s an optional suggestion that won’t hold up patch approval. If it’s untagged then it has to be fixed. We don’t have a tag for a general “good job” comment and our graybeards would probably raise hell if we started using emoji in comments...maybe I’ll start using [nice]
|
|
#
?
Apr 13, 2021 05:19
|
|
|
brand engager posted:Drives me crazy when a reviewer gets hung up on some implementation detail and wants to "wouldn't it be better to ___" without pointing out anything actually wrong with the one I already used. Comments on the merge request have now evolved into wanting to change how our mvvm stuff is implemented. That's already been like that for months and the MR change was just supposed to be fix for some concurrency bug.
|
|
#
?
Apr 13, 2021 05:40
|
|
|
asur posted:I strongly disagree with nitpicking and tagging them as nits. It just doesn't make sense, either it's an issue and should be fixed or it's not and there's no reason to bring it up as an issue. If you have a suggestion, then pick the instance, or one if there's multiple, and write the comment to clearly communicate that it's suggestion preferably with the review system not marking it as an issue if it allows it. This is a disappointing form of all or nothing level of pedantry. There's room for "I would like it if you did this but I get it if you think I should just gently caress off about it because we all have busy lives and differences of opinions" especially when it comes to stylistic decisions, naming, etc. For example "this might be cleaner if you did it this way but I can understand why you might prefer it this way"
|
|
#
?
Apr 13, 2021 06:25
|
|
|
I liked the emoji guide linked earlier because it had an explicit "usually better left unsaid" note for the nitpick-pickaxe emoji classification. It's not the end of the world if you do it, there is absolutely room for it on everyone's plate, but it's better if you don't.
|
|
#
?
Apr 13, 2021 07:19
|
|
|
I assume we are all just defining nitpick differently. Like I don’t consider complaining about bad variable names to be a nitpick since they live in the code forever and hurt readability and maintainability. The person getting reviewed would probably consider that nitpicking though.
|
|
#
?
Apr 13, 2021 14:48
|
|
|
My immediate group has two levels of maybe-actionable feedback that won't block a change approval. We use 'nit:' for minor objections that admittedly don't matter such as small naming or style issues. We use 'optional:' for things that we think would be improvements but aren't big enough issues to be blockers - i.e. "I would have done this this other way but they are equivalent so whatevs" or "such and such container would be more efficient but this isn't on a hot code path so it doesn't really matter that much".
|
|
#
?
Apr 13, 2021 15:40
|
|
|
Volmarias posted:This is a disappointing form of all or nothing level of pedantry. There's room for "I would like it if you did this but I get it if you think I should just gently caress off about it because we all have busy lives and differences of opinions" especially when it comes to stylistic decisions, naming, etc. For example "this might be cleaner if you did it this way but I can understand why you might prefer it this way" It's all or nothing because nits by definition are small and supposedly unimportant errors. If they're unimportant then stop wasting everyone's time nitpicking about them or they're actually important and are just issues. I also don't think it's nitpicking to ask someone to follow the style guide which is where a lot of these types of issues fall.
|
|
#
?
Apr 13, 2021 16:22
|
|
|
At my place, nits never mean “you can optionally do this if you want”, they mean “this does not meaningfully affect the code or functionality but I expect you to make this change anyway”.
|
|
#
?
Apr 13, 2021 16:32
|
|
|
Communication is the job. If you have established the norm that a particular channel of communication should not be used for nuanced or low-impact suggestions, then don't use it for nuanced or low-impact suggestions; if, on the other hand, the people reading your code reviews know that that in them they can expect to receive feedback items whose resolution will be left to their discretion, then you can go ahead and nitpick in them. Jesus, people. It's almost as though context is everything.
|
|
#
?
Apr 13, 2021 17:47
|
|
|
Bongo Bill posted:Communication is the job. If you have established the norm that a particular channel of communication should not be used for nuanced or low-impact suggestions, then don't use it for nuanced or low-impact suggestions; if, on the other hand, the people reading your code reviews know that that in them they can expect to receive feedback items whose resolution will be left to their discretion, then you can go ahead and nitpick in them. Jesus, people. It's almost as though context is everything. Sorry sir, I'm going to have to ask you to stop nitpicking.
|
|
#
?
Apr 13, 2021 18:15
|
|
|
Working on the mvvm redo. The reviewer is the lead dev so I knew it was probably a losing battle 
|
|
#
?
Apr 13, 2021 19:02
|
|
|
asur posted:It's all or nothing because nits by definition are small and supposedly unimportant errors. If they're unimportant then stop wasting everyone's time nitpicking about them or they're actually important and are just issues. I also don't think it's nitpicking to ask someone to follow the style guide which is where a lot of these types of issues fall. My org also uses [nit] as a way to say "Here's something I might do a little differently but it could come down to opinion." These sorts of things aren't stylistic, so they aren't covered by the style guide, and they are also not errors or total blockers. If I leave a nit, and the author makes my suggested change, great. If they don't, that's just fine and we all move on. I am not sure why you're getting so hung up on the vocabulary here. Would you prefer it if these sorts of comments were prefaced with "Suggestion:" instead?
|
|
#
?
Apr 15, 2021 00:04
|
|
|
Seems to me it's not about what to call it, but rather that this kind of feedback does more harm than good.
|
|
#
?
Apr 15, 2021 00:37
|
|
|
I’m beginning to hate it whenever I add nits to a PR. I’m going to stop doing it altogether I think.
|
|
#
?
Apr 15, 2021 00:40
|
|
|
dantheman650 posted:My org also uses [nit] as a way to say "Here's something I might do a little differently but it could come down to opinion." These sorts of things aren't stylistic, so they aren't covered by the style guide, and they are also not errors or total blockers. If I leave a nit, and the author makes my suggested change, great. If they don't, that's just fine and we all move on. We used code collaborator at a previous place where we had a strong separation of defect vs comment. Defects needed to be addressed (Fixed, or argued against) before the review could be closed, while the comment could be acted on or not.
|
|
#
?
Apr 15, 2021 00:41
|
|
|
Pollyanna posted:I’m beginning to hate it whenever I add nits to a PR. I’m going to stop doing it altogether I think. This has been my take away as well.
|
|
#
?
Apr 15, 2021 01:07
|
|
|
Whatever choice moves you further towards not caring about the job and what you do at it is the correct choice.
|
|
#
?
Apr 15, 2021 01:11
|
|
|
At my previous place, had a small group of us that just settled into the pattern of discussing larger important parts of the PR first to raise any showstoppers. Then when that's all resolved add all the nitpicky stuff in as comments then hit the "approve with comments" button. The dev can decide if they want to address all the trivial little things. Worked out well since for the most part we trusted they would do the nitpick stuff unless they had really strong feelings about it. This obviously relies on working with developers with some minimal amount of talent and communication skills, which I mean isn't really a given so your results may vary.
|
|
#
?
Apr 15, 2021 01:35
|
|
|
dantheman650 posted:My org also uses [nit] as a way to say "Here's something I might do a little differently but it could come down to opinion." These sorts of things aren't stylistic, so they aren't covered by the style guide, and they are also not errors or total blockers. If I leave a nit, and the author makes my suggested change, great. If they don't, that's just fine and we all move on. It is about the terminology to the extent that words have meaning and raising an issue against the code should mean that it needs to be fixed. I do think suggestions fall into a different category and it's preferable if your review tools allows differentiation between the two. If it doesn't then I guess your stuck prefixing something to the suggestion.
|
|
#
?
Apr 15, 2021 02:36
|
|
|
17bc39ef 2021-04-14 16:21.49: testing yaml change 91b3aa23 2021-04-14 16:43.27: testing yaml change 21ccde34 2021-04-14 17:19.11: testing yaml change 81f3199c 2021-04-14 17:42.16: gently caress azure devops
|
|
#
?
Apr 15, 2021 04:07
|
|
|
MisterZimbu posted:17bc39ef 2021-04-14 16:21.49: testing yaml change I've been begging the product team for a way to locally test yaml pipelines for literally years and they keep saying it's a good idea but they never implement it. Probably because they're all transitioning to the github actions team.
|
|
#
?
Apr 15, 2021 04:15
|
|
|
When I first started using Jenkins pipelines I went "oh neat it's just a Groovy script so it should be possible to run it locally to test things". I was very disappointed to discover that was not a use-case they'd even considered.
|
|
#
?
Apr 15, 2021 05:07
|
|
|
Just lol if each step in your Jenkins file isn’t just a call to a script.
|
|
#
?
Apr 15, 2021 05:10
|
|
|
asur posted:It is about the terminology to the extent that words have meaning and raising an issue against the code should mean that it needs to be fixed. I do think suggestions fall into a different category and it's preferable if your review tools allows differentiation between the two. If it doesn't then I guess your stuck prefixing something to the suggestion. Good thing it isn't called issue, but a comment.
|
|
#
?
Apr 15, 2021 07:24
|
|
|
New Yorp New Yorp posted:I've been begging the product team for a way to locally test yaml pipelines for literally years and they keep saying it's a good idea but they never implement it. "PRs welcome!  "
|
|
#
?
Apr 15, 2021 12:56
|
|
|
Plorkyeran posted:When I first started using Jenkins pipelines I went "oh neat it's just a Groovy script so it should be possible to run it locally to test things". I was very disappointed to discover that was not a use-case they'd even considered. Believe it or not you can do this by piping the job-generating groovy through a headless Jenkins, outputting to XML, then writing assertions against what should be there. No, there's no way to actually test what the job would do if it ran. Why would anyone want to do something as silly as that?
|
|
#
?
Apr 15, 2021 13:06
|
|
|
Gildiss posted:Whatever choice moves you further towards not caring about the job and what you do at it is the correct choice. Is this a sarcastic post or not? Sometimes I think I care too much about the job and get overly invested in debating details so it caught my attention 
|
|
#
?
Apr 15, 2021 13:13
|
|
|
Daviclond posted:Is this a sarcastic post or not? Sometimes I think I care too much about the job and get overly invested in debating details so it caught my attention Not sarcastic. Work sucks and is dehumanizing.
|
|
#
?
Apr 15, 2021 14:33
|
|
|
Solution: suck humans and deworkanize 
|
|
#
?
Apr 15, 2021 14:59
|
|
|
deworkanize and face to sucking
|
|
#
?
Apr 15, 2021 15:08
|
|
|
DoomTrainPhD posted:Just lol if each step in your Jenkins file isn’t just a call to a script. For a release/deploy pipeline you end up with a decent amount of stuff in the Jenkinsfile even if you strictly abide by the rule that anything which can be extracted to a separate script is. Even if the build step is just invoking a shell script, all the stuff around stashing and unstashing artifacts, parsing the build output to track warnings and perf test history, sending slack notifications when the pipeline completes, and just plain setting up the build nodes when you have a whole bunch of combinations of build platforms and target platforms ends up being a lot of code which would be nice to test.
|
|
#
?
Apr 15, 2021 17:01
|
|
|
Daviclond posted:Is this a sarcastic post or not? Sometimes I think I care too much about the job and get overly invested in debating details so it caught my attention Always a relief to leave a team or job and have all that tech debt just be someone else’s problem now.
|
|
#
?
Apr 15, 2021 17:49
|
|
|
I have the issue of feeling like I have to be an expert in every framework/technology and spend all my free time on side projects and classes or I’ll get fired It is I the junior developer
|
|
#
?
Apr 16, 2021 00:52
|
|
|
InevitableCheese posted:I have the issue of feeling like I have to be an expert in every framework/technology and spend all my free time on side projects and classes or I’ll get fired If you're a junior developer no one expects you to have knowledge for poo poo about poo poo, don't worry.
|
|
#
?
Apr 16, 2021 01:08
|
|
|
InevitableCheese posted:I have the issue of feeling like I have to be an expert in every framework/technology and spend all my free time on side projects and classes or I’ll get fired This is just another way for work to steal your time by expecting you to spend hours outside of work learning poo poo for work. If they want you to learn new technologies that learning should happen on the clock. If they wanted someone who already knew all of those frameworks (if they even existed when you were hired) then unless you lied on your resume that's their problem.
|
|
#
?
Apr 16, 2021 19:27
|
|
|
asur posted:It's all or nothing because nits by definition are small and supposedly unimportant errors. If they're unimportant then stop wasting everyone's time nitpicking about them or they're actually important and are just issues. I also don't think it's nitpicking to ask someone to follow the style guide which is where a lot of these types of issues fall. Nitpick is a label. It's a piece of data. It has no power by itself, it's about what you do with it. In the hands of an unsupportive culture, it's a way for whiny and insecure people to emasculate one another over code quality. In a supportive culture, it can actually communicate bidirectional trust in professional judgment between the author and the reviewer. Here are three kinds of non-blocking comments my team uses often. They are all implied to the extent that we don't really need to call out what we mean specifically:
|
|
#
?
Apr 17, 2021 04:19
|
|
|
Our process is that you absolutely cannot comment or question without *at least* putting a +/- 1 on the review. It's terrible and I advise against such a thing.
|
|
#
?
Apr 17, 2021 12:51
|
|
|
|
| # ? May 9, 2024 18:29 |
|
|
I don't get the opposition to "what you did is acceptable but I would have done X" comments. I like getting them because the suggestions usually are better than what I wrote and when I disagree with them I just click resolve conversation and move on. "If they're unimportant then stop wasting everyone's time nitpicking about them or they're actually important and are just issues." is just a weird false dichotomy. There's a lot of things which long term will make maintenance more difficult, but aren't functional problems and individually are insignificant. It would be ridiculous to block shipping a bug fix because the code has some misleading variable names, but if they're touching the code some more to fix some more important problems then they might as well pick some better names at the same time too.
|
|
#
?
Apr 17, 2021 17:51
|
|