My team typically prefixes small comments like that (assuming they're not caught by a linter) with "nit: thing could be more descriptive" so that the reviewee knows that these are minor things. Then when there's a comment that doesn't have a nit, they know that's more important feedback to consider.
If a comment can be ignored or is optional, then it's a waste of time adding it. The whole point of the video is these "nit picks" should and can be automated away
@@TheStruders a linter can't tell you that your variable isn't describing the right thing. saying green = yellow isn't something a linter is going to find.
@@TheStruders a comment might be optional if, time does not allow or the implementor has a good reason not to do it. For example, lets say were moving in this new direction and any new code should be migrated, you as a senior might want to share that knowledge and inform the implementor but not enforce because maybe the refactor is just too big to do at that point. You tone implies you have a hard and fast rule, which is almost certainly wrong some of the times!
Raymond Hettinger gave a talk "beyond PEP8" for Python. There is a great experiment he shows to demonstrate the distraction problem (you need to count basketballs on screen).
Well, most developers will think about PR reviews as a chore. Everyone wants to code but not everyone wants to review other's code. So this might be a mentality problem too. I've left 12 (it was 12 btw :D) comments, I did my part. But also, I wouldn't say that the review given sucks in such case. Yes, you lost focus as a reviewer of what's important because you've been distracted by smaller issues, but I'd blame it on the person creating the PR. Even as a junior, are you really not able to follow naming conventions that are already established in the code base? I don't want to introduce a thousand linter rules for each case. I want people be able to follow established project styles.
Don't get me wrong sir, I have just idea, that the way you talk about those CR rules relates to usage of JS and VSCode. I apologize, I did not mean it wrong or bad, just idea to share. PS. I actually like how reflecting (on the other hand) you are towards your code.
Yeah that's fair, JS is so I unopinionated that there are a lot more syntax "mistakes" it allows. A more strict language simply wouldn't compile this kind of thing. The video itself was inspired by a PR I reviewed that had lots of "nitpick" comments but no one spotted a major issue in business logic. It's a bit contrived as I can't show actual code from my employer's app. Thanks for the thoughtful feedback!
lf you have more than 10 rules / conventions in your code base and people can't get them right by hand it's a smell your codebase is entertaining OCD and will never have good reviews that catch bugs or feature spec mismatches. Linters just legitimize stupid shit like single quote vs double quote, never align assignments, always have a space after a round bracket but not a square one and a litany of other I-Never-Learned-What-Matters stuff.
Yes, code reviews are great for devs of all skill levels to learn new things, teach others but last but not least to help devs be aware of the trends in the codebase.
My team typically prefixes small comments like that (assuming they're not caught by a linter) with "nit: thing could be more descriptive" so that the reviewee knows that these are minor things. Then when there's a comment that doesn't have a nit, they know that's more important feedback to consider.
Same
If a comment can be ignored or is optional, then it's a waste of time adding it. The whole point of the video is these "nit picks" should and can be automated away
@@TheStruders a linter can't tell you that your variable isn't describing the right thing. saying green = yellow isn't something a linter is going to find.
@@TheStruders a comment might be optional if, time does not allow or the implementor has a good reason not to do it.
For example, lets say were moving in this new direction and any new code should be migrated, you as a senior might want to share that knowledge and inform the implementor but not enforce because maybe the refactor is just too big to do at that point.
You tone implies you have a hard and fast rule, which is almost certainly wrong some of the times!
This guy right here == gold advice
i have absolutely seen the same problem at my previous job.
love your content. please upload more knowledge.
Raymond Hettinger gave a talk "beyond PEP8" for Python. There is a great experiment he shows to demonstrate the distraction problem (you need to count basketballs on screen).
Well, most developers will think about PR reviews as a chore. Everyone wants to code but not everyone wants to review other's code. So this might be a mentality problem too. I've left 12 (it was 12 btw :D) comments, I did my part.
But also, I wouldn't say that the review given sucks in such case. Yes, you lost focus as a reviewer of what's important because you've been distracted by smaller issues, but I'd blame it on the person creating the PR. Even as a junior, are you really not able to follow naming conventions that are already established in the code base? I don't want to introduce a thousand linter rules for each case. I want people be able to follow established project styles.
if your *many* tiny rules can't be introduced by a linter & formatter then you are just creating arbitrary rules that follow no pattern...
@@axisaligned9799 Yes, definitely, we can go to either extreme. Having too many arbitrary rules is the other end of the spectrum.
Pity you didn't address the most import thing here: fetching data in useEffect
Don't get me wrong sir, I have just idea, that the way you talk about those CR rules relates to usage of JS and VSCode. I apologize, I did not mean it wrong or bad, just idea to share.
PS. I actually like how reflecting (on the other hand) you are towards your code.
Yeah that's fair, JS is so I unopinionated that there are a lot more syntax "mistakes" it allows. A more strict language simply wouldn't compile this kind of thing.
The video itself was inspired by a PR I reviewed that had lots of "nitpick" comments but no one spotted a major issue in business logic. It's a bit contrived as I can't show actual code from my employer's app. Thanks for the thoughtful feedback!
lf you have more than 10 rules / conventions in your code base and people can't get them right by hand it's a smell your codebase is entertaining OCD and will never have good reviews that catch bugs or feature spec mismatches. Linters just legitimize stupid shit like single quote vs double quote, never align assignments, always have a space after a round bracket but not a square one and a litany of other I-Never-Learned-What-Matters stuff.
do people want to spend time to review code?
Yes, code reviews are great for devs of all skill levels to learn new things, teach others but last but not least to help devs be aware of the trends in the codebase.
Great video
I prefer javascript, never saw the value in typescript personally.
Wow
@@SonAyoD I see we have a typescript fan boi here.
Why don't you feel the value in type safety?
@@devoiddude im just interested in your reasoning. I could never build any medium to large system without type safety.
@@devoiddude We call them 'engineers'