Consider applying black to format the code #110
Conversation
There was a problem hiding this comment.
To conform with PEP8, all lines should be limited to 79 characters.
As a matter of personal taste, I prefer using single quotes to double quotes for strings. Is it possible to set Black to single quotes?
Finally, these changes break the type annotation syntax. (https://travis-ci.org/n1analytics/clkhash/jobs/365941315)
To quote directly from the intro to black:
So no. You don't get personal taste... Pick any color you like I've updated with the default settings which appear to use slightly more than 79 characters which is probably a sensible value. |
clkhash/backports.py
Outdated
| @@ -22,7 +22,9 @@ def __int_from_bytes(bytes, byteorder, signed=False): | |||
| :param byteorder: Either `'big'` or `'little'`. | |||
| """ | |||
| if signed: | |||
| raise NotImplementedError("Signed integers are not currently supported in this " "backport.") | |||
| raise NotImplementedError( | |||
| "Signed integers are not currently supported in this " "backport." | |||
There was a problem hiding this comment.
Black doesn’t recognise that these will be merged into one string so it doesn’t remove the superfluous " “.
clkhash/backports.py
Outdated
| @@ -93,5 +102,6 @@ def _p2_unicode_reader(unicode_csv_data, dialect=csv.excel, **kwargs): | |||
|
|
|||
|
|
|||
| unicode_reader = ( | |||
| _p2_unicode_reader if sys.version_info < (3, 0) else csv.reader # Python 2 with hacky workarounds. | |||
| _p2_unicode_reader if sys.version_info | |||
| < (3, 0) else csv.reader # Python 2 with hacky workarounds. | |||
There was a problem hiding this comment.
I find this much less readable than the previous form.
| keys, | ||
| k, | ||
| l, | ||
| encoding, # type: Iterable[str] # type: Sequence[bytes] # type: int # type: int # type: str |
There was a problem hiding this comment.
MyPy compatibility is broken here.
| keys, | ||
| k, | ||
| l, | ||
| encoding, # type: Iterable[str] # type: Sequence[bytes] # type: int # type: int # type: str |
There was a problem hiding this comment.
Ditto regarding Mypy compatibility.
| keys, | ||
| k, | ||
| l, | ||
| encoding, # type: Iterable[str] # type: Sequence[bytes] # type: int # type: int # type: str |
There was a problem hiding this comment.
Ditto regarding Mypy compatibility.
clkhash/bloomfilter.py
Outdated
| dataset, keys, schema # type: Iterable[Sequence[Text]] # type: Sequence[Sequence[bytes]] # type: Schema | ||
| dataset, | ||
| keys, | ||
| schema, # type: Iterable[Sequence[Text]] # type: Sequence[Sequence[bytes]] # type: Schema |
| 'parameter "l" has to be a power of two for the BLAKE2 encoding, but was: {}'.format( | ||
| l | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Agree - it looks better with the longer line length - 8254286#diff-f36426d58cf2623d909f2f77aae79799R183
clkhash/clk.py
Outdated
| from typing import AnyStr, Callable, Iterable, List, Optional, Sequence, TextIO, Tuple, TypeVar | ||
| from typing import ( | ||
| AnyStr, Callable, Iterable, List, Optional, Sequence, TextIO, Tuple, TypeVar | ||
| ) |
| case != self._DEFAULT_CASE | ||
| or min_length != self._DEFAULT_MIN_LENGTH | ||
| or max_length is not None | ||
| ) |
|
Ok thanks for the review. I think for now we can safely say we can't use black without changing how we annotate types for mypy. |
See #99
This PR is simply the diff after running:
This is more for discussion that merging right now. I don't know why I chose a line wrap length of 120 - maybe my wide monitor influenced me...