Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initial guidance on using type annotations #49

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
Draft
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 24 additions & 3 deletions process/Style-guide.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ variations and expansion spelled out in the sections below.

## Compatibility

All new log2timeline code should be compatible with Python 3.5+.
All new log2timeline code should be compatible with Python 3.6+.

## Indentation

Expand All @@ -37,7 +37,6 @@ Type | Public | Internal
---- | ---- | ----
Functions | **CapWords()** | **_CapWords()** (protected) and **__CapWords()** (private)


## Linting

log2timeline uses pylint 2.4.x to enforce some additional best practices to keep the source code more readable. These are:
Expand All @@ -48,6 +47,28 @@ Other things to keep in mind when using Pylint:
* Use textual pylint overrides e.g. "# pylint: disable=no-self-argument" instead of "# pylint: disable=E0213". For a list of overrides see: http://docs.pylint.org/features.html
* The canonical pylint configuration for log2timeline project is generated by l2tdevtools.

### Type annotations

* Do not use `from __future__ import annotations` since it is not compatible with Python 3.6
* For importing typing see: http://google.github.io/styleguide/pyguide.html#31912-imports-for-typing
* Define type annotations as strings so `'int'` instead of `int`. This prevents having issues where types are self referencing
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure it's worth adding all these characters for the small number of classes that reference themselves. Let's just do this on the rare cases it's necessary.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So http://google.github.io/styleguide/pyguide.html#31912-imports-for-typing states:

Conditionally imported types need to be referenced as strings, to be forward compatible with Python 3.6 where the annotation expressions are actually evaluated.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recommend us sticking with strings for now to be consistent

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about instead we wait until we've dropped support for python 3.6, as this string ugliness isn't required in Python 3.7+ https://www.python.org/dev/peps/pep-0563/

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So type annotations will remain somewhat "ugly" with Python 3.7 and later as well, but we can postpone adopting them once we are able to drop 3.6 support.

* Type annotate function or method arguments and return types
```python

def _GetNumberOfDaysInYear(self, year: 'int') -> 'int':
"""Retrieves the number of days in a specific year."""
```

* Type annotate instance attributes
```python

def __init__(self, timestamp: 'Optional[int]' = None) -> 'None':
"""Initializes a WebKit timestamp."""
super(WebKitTime, self).__init__()
self._precision: 'str' = definitions.PRECISION_1_MICROSECOND
self._timestamp: 'Union[int, None]' = timestamp
```
* Limit the usage of `Optional` to function or method arguments, use `Union` in other cases

### Function and method arguments

Expand All @@ -56,7 +77,7 @@ Other things to keep in mind when using Pylint:
```python

def SomeMethod(self, alternate=False, secondary=True):
pass
return

# No:
SomeMethod(True, False)
Expand Down