Python Code Review Guidelines

[maximiliankaul]

Some points in no particular order:

• The code does not use .first() -> use .firstOrNull()
• The code does not use
  • as (only with a very good reason -> rule of thumb "don't")
  • as? (use assertIs<>() when writing tests)
  • is (only for filters)
• The code does not assertEquals on .refersTo -> use assertRefersTo
• The code does not assertEquals on .invokes -> use assertInvokes
• The code uses a when clause when checking for multiple possible types over an if and logs an error/warning when unexpected types occur instead of ignoring the unexpected.
• All (public) functions have a doc string
• Access is as restricted as practical
• No IDE / Sonar cube errors / warning / code quality complaints.
• Prefer using named parameters when calling a function unless it's super clear what the parameter does (e.g. foo(rawNode) is not less readable than foo(rawNode = rawNode) and acceptable -> be smart about when not to use named parameters).

[oxisto]

Adding this here for a lack of a better place:

We have the following policies in place for pull requests:

• We need at least 1 reviewer approval
• All build pipelines must succeed
• The test coverage is in fact not enforced, but it SHOULD be met
• All comments need to be resolved

When reviewing a pull request you have three options:

• Request changes: Please only do this if you really really OPPOSE the way this pull request works. The reason behind this is that GitHub forces you to actively re-request a review and if that doesn't happen, you need to manually "dismiss" this negative review, even if someone else approves this. So this is blocking progress unintentionally if it is just smaller changes
• Just leave comments without approve: You can do that if you feel that this PR still needs some work, especially detailed in the comment or if you think that you are not confident enough to click "approve"
• Approve (with comments): If you have only smaller nitpicks (e.g., maybe add a doc string here), but you are generally fine with the PR, you can approve and leave comments. As long as the comments are not resolved the merge will be blocked.

As for resolving comments, we have the following guideline
If it's an obvious fix, e.g., please rename this, or add documentation, etc.: The one who did the PR can resolve them. If its a more complicated fix or something to discuss, it would be polite to leave the resolution to the one who created the comment.