Pull request review mistakes
blog.scottnonnenberg.com
blog.scottnonnenberg.com
(However, more issues are typically introduced by manual conflict resolution, since we humans are also easily confused when doing it. Both are a problem relatively independent of language, although some specific instances might be caught by a compiler or tests. If the latter don't catch it, it may be a sign that some tests are missing.)
Be careful.
Coding style (apart from formatting) can be crucial for understandability, so it's important to have a critical look at it when reviewing. For instance, consider the following snippets:
void myMethod(FooBar arg) {
if (arg != null) {
arg.foo(something);
for (int i = 0; i < arg.bars().size(); i++) {
arg.bars().get(i).baz();
}
}
}
void myMethod(FooBar arg) {
if (arg == null)
return;
arg.foo(something);
arg.bars().forEach(baz);
}
Both of these (if vs guard clause, loop vs each) are enforced by the Rubocop linter in Ruby.
It's good and nice that there is one linter for a particular language that catches 100 % of these issues in an example, but that doesn't do much to change the fact that most linters for most languages are unable to do this, and that there are quite some instances of these decisions where it is doubtful that any linter would be able to lint it.
In the long run improving the linter is better for everyone. The examples given by the OP as infeasible are feasible.
Focus on the design over trivial style issues. Your style guide should be 95% covered by the linter. If it isn't, fix your linter.
> Broadly missing a point
My points is: Underestimating static analysis will lead you to the wrong conclusions.
Such issues are rarely style issues. They are more in the design territory where the focus of code reviews should be.
This is doubly true when delinting and styles are automatic via git hooks.
At my job my coworker and I did a major refactor on part of our frontend codebase. Probably 2,000 lines of code were changed and the only comment on the PR was for additional unit tests on one method we had touched on the backend. We brought it up multiple times in standup that this really needed to be reviewed properly and we got radio silence. The PR was just passed through.
Your entire platform really does matter, it's important to make sure all things work. Not just what you understand.
The obvious ones I'd discard are the ones that don't use a VCS (I don't think I'd take a non-git one either, and CSV is definitely a no-go).
Sometimes it's interesting that no mention of tests is made when asking these questions, or style guides maybe. It really give you a taste of the "code culture" for that employer.
Conversely, it's just so easy to add an if statement there or any early return here, and over time you've got a 400-line method with like 100 branches and an Avagadro's number of possible paths. But you look at a two-line diff and say, "Hey, this looks reasonable."
This one is usually an automated task. Everyone agrees on a coding style, configures his/her development environment and first time you work on a non-std file you convert and commit it (most things are done by your editor/IDE, if configured correctly).
> If the code works and is readable, who cares if it varies somewhat from similar code.
Me. I regularly have to clean up after lone warriors who talk like you have "delivered features".
A coding style does not represent some universal truth about code esthetics: On one hand it allows programmers to read code with a well defined set of expectations, on the other hand it helps prevent some of the big annoyances for reviewers.
The point is not to like some specific style but to adhere to it. It takes you 5 minutes to adapt your own code to the project's coding style, it will take others 10 minutes to do so and, unfortunately the most common case, it will take hours over a single year when nobody fixes your coding style and everybody who has to touch your code trips over the same things.
Check it out: https://runnable.com/ or contact me if you have any questions.
This also sounds very specific (probably using apache+cgi?). It won't really work for most techs out there.
We're not using the apache cgi plugin for python to resolve dependencies, just packages relative the the application root. I think most languages that can import dependencies can import relative dependencies.
Although my specific setup may not work for everyone, I suspect that there is an equally simple solution for most projects. Containerization is definitely a long-term goal, but it is not prerequisite for automated deploys. The things you have to change to get deploys to work on an existing server will also be useful for containerization.