4 ms·

> * Don't give orders ("Do this", "Do that") to your colleagues, since they are not your employees, and it doesn't help promote learning & growth by not exposin

> * Don't give orders ("Do this", "Do that") to your colleagues, since they are not your employees, and it doesn't help promote learning & growth by not exposing the reasoning behind the demand; I think this depends a lot. I do "readability" reviews at my company to make sure people apply good coding standards and adhere to the coding guidelines that we have. A lot of these are very "mechanical" in nature and when I am reviewing changes that have obvious coding standard mistakes I will write comments like a "linter" would. One of the most common examples: > Use descriptive ("Uses") rather than imperative ("Use") style for function docstrings. See: <url-explaining-docstring-guidelines> I think this type of "giving orders" in reviews is fine as long as it's clear this is an obvious misuse/misunderstanding/miss of the readability guidelines. For any other kind of comment I'll phrase it differently ("Is there a reason why you did X? I think doing Y might be more appropriate because <reasons>, what do you think?", etc) but for stuff that linter should've caught I don't think it's necessary to be verbose and "sweeten" the comments too much.

devnulll 4y ago
> I do "readability" reviews at my company to make sure > people apply good coding standards and adhere to the > coding guidelines that we have. Consider avoiding the entire formatting arguments as part of the code review process. There are excellent linters for every language that can run as part of the check-in process. GitHub + Actions can automate every aspect of this using all turkey stuff. Automating where possible avoids all the arguments, actually enforces standards, and saves time for everyone. Using "Standard" linting rules (aka: The defaults) also help promote good practices for everyone.
- Thorrez 4y ago
  Where I work there is a readability review like mason55's (probably the same company). Most of the comments I see are not about formatting. They're more like "Take parameters by std::string_view instead of const std::string&" or "Use a reference member instead of a pointer member if it'll never be null." Applying a linter to a giant company-wide monorepo can be difficult because of decades-old code that predates certain style rules, or some teams that might have created their own rules. Even if the linter only applies to diffs, a small change to a line can trigger a lint warning on that line, and having a ton of those can be annoying.
- Morgawr 4y ago
  > Consider avoiding the entire formatting arguments as part of the code review process. That is not going to help me if people send code to review that has formatting problems. We catch a lot of stuff using automated tools, but there's some stuff that you cannot catch. Some people declare every variable as global rather than private, and then I have to follow up with "This variable/constant should be private, any reason you marked it as external?" etc. You can't just automate everything, and we also have our own specific code formatting guidelines that a lot of tools don't automatically detect by default. I have raised some of these points to our toolchain and engineer productivity teams but it's not my job to dig deep into our tooling and automation so until there's better support in our internal tools (we don't use github + actions) to detect that, I'll keep pointing out any formatting mishap that gets through our linters.

devnulll 4y ago

> I do "readability" reviews at my company to make sure > people apply good coding standards and adhere to the > coding guidelines that we have. Consider avoiding the entire formatting arguments as part of the code review process. There are excellent linters for every language that can run as part of the check-in process. GitHub + Actions can automate every aspect of this using all turkey stuff. Automating where possible avoids all the arguments, actually enforces standards, and saves time for everyone. Using "Standard" linting rules (aka: The defaults) also help promote good practices for everyone.

Thorrez 4y ago
Where I work there is a readability review like mason55's (probably the same company). Most of the comments I see are not about formatting. They're more like "Take parameters by std::string_view instead of const std::string&" or "Use a reference member instead of a pointer member if it'll never be null." Applying a linter to a giant company-wide monorepo can be difficult because of decades-old code that predates certain style rules, or some teams that might have created their own rules. Even if the linter only applies to diffs, a small change to a line can trigger a lint warning on that line, and having a ton of those can be annoying.
Morgawr 4y ago
> Consider avoiding the entire formatting arguments as part of the code review process. That is not going to help me if people send code to review that has formatting problems. We catch a lot of stuff using automated tools, but there's some stuff that you cannot catch. Some people declare every variable as global rather than private, and then I have to follow up with "This variable/constant should be private, any reason you marked it as external?" etc. You can't just automate everything, and we also have our own specific code formatting guidelines that a lot of tools don't automatically detect by default. I have raised some of these points to our toolchain and engineer productivity teams but it's not my job to dig deep into our tooling and automation so until there's better support in our internal tools (we don't use github + actions) to detect that, I'll keep pointing out any formatting mishap that gets through our linters.