3.1 KiB
Code Review Guidelines
PyGuide
General PR process
- The scope of PR should be simple, unique and well-defined. PR should not contain unrelated changes
- Approve PR only if you are sure about the scope
- Be respectful and reply asap
- Avoid spending too much time on trivial changes
- Avoid premature optimisation
- For community PR, check CLA by typing
recheckcla
- Ask for jina env if needed by
jina -vf
- Suggest changes and check back ASAP. Get PR merged soon
- Suggest adding appropriate documentation for new features
- Don't forget to praise when PR is ready with something like LGTM
TLDR for pyguide
- Variables names need to be informative. No k, v or x.
- Use standard import order
- Define global variables in CAPS
- Use appropriate underscore for variable naming (leading, lagging, single, double)
- Add docstrings
- Add type hints
- Use codetags like #TODO in code wherever needed
- Raise appropriate exceptions
- Don't rename functions & arguments exposed to the user unless necessary. Have appropriate depreciation strategy
Jina specific
-
Use this to create random ports for servers
from jina.helper import random_port
-
You can keep function specific imports inside functions
-
Jina ENV variables should start with JINA_
-
For type hints which require import, you can do it like this
if False:
from PIL import Image
def func(arr: 'Image'):
from PIL import Image
...
Adding tests
- Add new unit & integration test if you add a new feature
- Use pytest tmpdir fixture for temporary directories
- Check test locally before pushing
- If a
Flow()
needs to be built in the test and is not a test of the Flow module, the test should go under/tests/integration
Core
Single Responsibility Principle
Do not place more than one responsibility into a single class or function. Instead, refactor into separate classes and functions.
Open Closed Principle
While adding new functionality, existing code should not be modified. New functionality should be written in new classes and functions.
Liskov substitutability principle
The child class should not change the behavior (meaning) of the parent class. The child class can be used as a substitute for a base class.
Interface segregation
Do not create lengthy interfaces. Instead, split them into smaller interfaces based on the functionality. The interface should not contain any dependencies (parameters) which are not required for the expected functionality.
Dependency Injection
Inject dependencies instead of hard-coding them.