Firtst thoughts about guide for reviewers #2688 #2689
Conversation
de-jcup
force-pushed
the
feature-2688-guidelines-for-reviewers
branch
2 times, most recently
from
e696e3b
to
072cd15
Compare
de-jcup
force-pushed
the
feature-2688-guidelines-for-reviewers
branch
from
072cd15
to
4345a53
Compare
| === General | ||
| ==== Understand and reflect | ||
| - *PR content check* + | ||
| A PR must have at least one issue which is referenced by the PR. T |
| - maintainability (it shall be clear how the code works and how a change can be made) | ||
| - security aspects | ||
| - no typical flaws | ||
| - common style |
There was a problem hiding this comment.
Just out of curiosity, Why aren't Code Style Tools like CheckStyle integrated to enforce Unified Code Style?
One Could also run it as pre-commit hook and the CI should enforce the Style is acc. to MB Standards.
There was a problem hiding this comment.
SpotBugs & PMD could also be beneficial here, what do you think?
There was a problem hiding this comment.
We currently use spotless to check for correct formatting.
But we could integrate other tools like PMD,Checkstyle or SpotBugs in future - good point.
I was thinking about using arch unit as well in the past, but I decided to simply write some extra unit tests to test some parts (e.g. domains cannot communicate directly with each other to keep DDD working). But maybe arch unit will come at some point of time.
| - unit tests | ||
| * for all/most introduced classes, but... | ||
| * not for standard CRUD parts done by Spring automatically | ||
| * not for simle beans only having getter/setters |
There was a problem hiding this comment.
Typo: simle -> simple
| :toc: left | ||
| == Reviewers guide | ||
|
|
||
| At {sechub} we have some quality aspects for pull requests. |
There was a problem hiding this comment.
Once cloud also add a PR Template on the Repo level which includes some checkboxes to ensure the Author thought about some crucial points (Based on this Doc).
I have done it my current projects at work and it really helped me reduce some noise around Code Reviews and PRs.
See: PR Template Guide
|
@Abdullah-Benomar-Shahen : first of all: thank you for your ideas and your suggestions. This was only a first draft, means only an idea. IMHO it was too much content (documentation) from myself about a PR review - normally just a "rethink if all parts are done and if there is something missing" could be enough. On the other hand your suggestions are very wellcome . Currently I have many other issues so this will still need some time. |
This PR