forked from rucio/documentation
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Developer Documentation: Brief style guidelines and instructions for …
…checking style rucio#287
- Loading branch information
Showing
2 changed files
with
172 additions
and
0 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,171 @@ | ||
--- | ||
id: dev-style-guide | ||
title: Style Guide | ||
--- | ||
|
||
**TL;DR** - Install the provided pre-commits, follow their recommendations | ||
|
||
# General Style | ||
## Spacing | ||
* Include 2 lines between functions outside of classes. | ||
* Include 1 line between class methods. | ||
* Include 2 lines between the import block and main code body. | ||
|
||
## White Space | ||
* Puncuation requires trailing whitespace except at the end of a line, and no leading whitespace. | ||
* Assignement (`=`) or comparision (`==`, `<`, `in`, etc) required surrounding white space, except when used in default definitions. | ||
* Colons in dictionaries or type annotations should have trailing, but not leading whitespace. | ||
|
||
## Imports | ||
* Never import using `from x import *` | ||
* Order alphabetically, then seperated into sections for internal and external dependencies. Group internal imports at the end of the block, and group imports from the same external package. | ||
* Order modules such that `import {packageA}` is before `from {packageB} import {Module}` | ||
* Do not import whole packages when single modules would suffice. | ||
* Unused imports must be removed. | ||
* When a large number of individual imports form a single package/module is required, group them together with `()` and separate them on their own lines. | ||
* When importing a module specifically for type checking (e.g. a core module that may not be included in every distribution of rucio, a type from SQLAlchemy), contain them in a block using | ||
```python | ||
from typing import TYPE_CHECKING | ||
if TYPE_CHECKING: | ||
from {package} import {module} | ||
``` | ||
|
||
[`ruff's isort` implimentation](https://docs.astral.sh/ruff/faq/#how-does-ruffs-import-sorting-compare-to-isort) handles import sorting in the rucio `pre-commit`s. | ||
|
||
#### Examples: | ||
```python | ||
# Wrong | ||
import rucio | ||
from datetime import * | ||
import os | ||
|
||
# Right | ||
import os | ||
from datetime import datetime, timedelta | ||
|
||
from rucio.core.did import add_did | ||
``` | ||
|
||
```python | ||
# Wrong | ||
from packageA import moduleA, moduleB, moduleC, moduleD, moduleE, moduleF, moduleG, ... | ||
|
||
# Right | ||
from packageA import ( | ||
moduleA, | ||
moduleB, | ||
moduleC, | ||
moduleD, | ||
moduleE, | ||
moduleF, | ||
moduleG, | ||
... | ||
) | ||
``` | ||
## Docstrings | ||
TBA | ||
|
||
# Testing Guide | ||
* Use fixtures (found in the tests/conftest.py) or temporary object factories (tests/temp_factories.py) instead of making bare instances of rucio objects. | ||
* Only write tests derministically. Randomness produces [flaky tests](https://docs.pytest.org/en/7.1.x/explanation/flaky.html). | ||
* Only write tests that are "stand alone" - tests should be entirely self-contained besides for the beforementioned fixtures and factories. | ||
* If a test requires a configuration file changed, [use a fixture to modify a mock-configuration file.](https://github.com/rucio/rucio/blob/master/tests/conftest.py#L510) | ||
* If a test can interfer with another test (use the same database table, interact with a queue), mark it as `noparallel`. | ||
|
||
# SQLAlchemy Query Guide | ||
Rucio uses a custom SQLAlchemy style. | ||
Each major clause or operation is given a new line, and a level of indentation is added for each operation within a part of a query. | ||
|
||
When queries as executed, perfered output types are either scalars or single values, to avoid using an index to select an element out of a query. | ||
|
||
Shorter statements can be written on a single line. | ||
|
||
> **Note** - When using SQLAlchemy to make query data model, it is best practice to call the code executed (the `select` statment or similar) `statement` or `stmt`. The actual `query` is the result of the statement's execution. | ||
### Examples | ||
#### Simple Select | ||
```python | ||
statement = select( | ||
models.Table.ColumnA, | ||
models.Table.ColumnB, | ||
) | ||
query = session.execute(statement).scalars() | ||
for column_a, column_b in query: | ||
... | ||
``` | ||
#### Select with Join | ||
```python | ||
statement = select( | ||
models.Table1.ColumnA, | ||
models.Table1.ColumnB, | ||
models.Table2.Column2 | ||
).join( | ||
models.Table2, | ||
(models.Table1.keyA == models.Table2.keyA) & (models.Table1.keyB == models.Table2.keyB) | ||
) | ||
query = session.execute(statement).scalars() | ||
for column_a, column_b, column_c in query: | ||
... | ||
``` | ||
|
||
#### Multiple Conditions | ||
```python | ||
statement = select( | ||
models.Table1.Column_a, | ||
models.Table1.Column_b, | ||
models.Table1.Column_c, | ||
).where( | ||
and_( | ||
models.Table1.Condition1 == True, | ||
models.Table1.Condition2 != None, | ||
models.Table1.Condition2 < {Different Statement} | ||
) | ||
) | ||
query = session.execute(statement).scalars() | ||
for column_a, column_b, column_c in query: | ||
... | ||
``` | ||
|
||
#### Checking existance/Single Value | ||
When using the query to ensure that table has been populated, or only a single result is required, use either `session.execute().one()` or `session.execute().scalar_one()`. | ||
|
||
```python | ||
statement = select(model.Table.Column).where(Condition) | ||
try: | ||
session.execute(statement).one() | ||
except NoResultFound as e: | ||
... # Handle the case where nothing exists | ||
``` | ||
|
||
```python | ||
statement = select(model.Table).where(Condition) | ||
query = session.execute(statement).scalar_one() | ||
foo(query.Column1, query.Column2) | ||
``` | ||
|
||
# Pre-commits | ||
Rucio uses [`flake8`](https://flake8.pycqa.org/en/latest/) as a linter, [`ruff`](https://docs.astral.sh/ruff/) as a formatter, a custom write-space remover, | ||
and a script to verify a uniform file-header format. | ||
Please use these before submitting a pull request. | ||
|
||
The Rucio repo provides a `pre-commit` that does this automatically. | ||
Install it with the below commands. | ||
|
||
```shell | ||
pip install pre-commit | ||
pre-commit install | ||
``` | ||
|
||
# Github Actions | ||
Code style is checked during a pull request with a github action. | ||
The action checks the header and type annotations (including a count and veracity). | ||
More information about type annotations can be found [here](./type_annotation_guide.md). | ||
These checks can also be run locally using | ||
|
||
```shell | ||
tools/count_missing_type_annotations_utils.sh | ||
tools/run_pyright.sh generate {report_output_path.json} | ||
``` | ||
The first action will raise an error if your commits introduce more un-annotated types than it solves, | ||
and the second ensures the added types are consistent with the rest of the codebase. | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters