Share this comment
Great inspo for the PR review tool we recently debuted at GitClear (gitclear.com/help/pull_…), thanks for posting this!
One thing that left me curious was what sort of "Static Analysis" is seen as desirable by Critique? I observe "Static Analysis" being mentioned as desirable both by the Reddit person and the Mozilla developers.
To some ex…
Great inspo for the PR review tool we recently debuted at GitClear (https://www.gitclear.com/help/pull_request_code_review_quality_overview), thanks for posting this!
One thing that left me curious was what sort of "Static Analysis" is seen as desirable by Critique? I observe "Static Analysis" being mentioned as desirable both by the Reddit person and the Mozilla developers.
To some extent it seems like the interest in static analysis is to help keep devs in line with the style guides & best practices, but I would imagine that a linter / IDE would be the better way to enforce code conventions (while the code is being written, instead of waiting for review). Does anyone have examples of the valuable Static Analysis that Critique performs? If this analysis could be handled by a linter, why wouldn't that be preferred by Google? Too heterogeneous a dev IDE ecosystem?
GitClear looks great - thanks for sharing!
Static Analysis at Google covers a lot more than just linting (it includes linting). I'd recommend this publication by Google regarding static analysis for more info: https://storage.googleapis.com/pub-tools-public-publication-data/pdf/3198e114c4b70702b27e6d88de2c92734c9ac4c0.pdf
Wow, what a goldmine of specific, interesting detail!! Thanks so much for sharing this, and for giving the world a good target to aim for in building the most useful possible PR review tool. I've long been mystified by how Github, Gitlab and Bitbucket are all effectively identical in their PR review experience. It's great to learn that there are tools that go beyond the boring GitHub implementation. So far I think my favorite features I've seen listed are the "diff each file only since the version that was last reviewed," "how much did time to run tests change," and "suggest code that addresses this comment." Juicy stuff, thanks again for all this inspo!
You're right that the static analysis feature partially serves this function (although it's also for more expensive checks that wouldn't be convenient to run locally).
Google is not heavily prescriptive about the tool you use for actually writing code. Many of the IDEs and editors that people do use, particularly the ones that have strong internal support, have style checks built into them, and it's obviously much more efficient to expose / correct these things at the time that code is being authored.
However, if an engineer wants to, they can use `ed` to just edit the code as raw text. What the static analysis mechanism does is to provide a consistent leveling bar for every piece of code that's committed to make sure that the same standards are applied uniformly irrespective of the local development environment.
Yep, that makes sense, thanks for sharing!
Only one of the presubmit checks is that there are no linter errors, but there is also slow running integration tests and tests on the transitively affected code, config diffs and lots of little project specific ones, like js payload size.