PEP8 (WIP) #146
PEP8 (WIP) #146
Conversation
|
|
||
| # :see: http://stackoverflow.com/a/2073599/ | ||
| from pkg_resources import require | ||
| from pkg_resources import require # noqa |
There was a problem hiding this comment.
❔ I could see why the wildcard imports would need a # noqa, but does this import need one as well?
There was a problem hiding this comment.
require is not used. As I just read this is a warning of flake, not a PEP8 violation. But, if we include flake8 into travisCI this would be nice to get rid of. One way of handling this would be to explicitly export imported classes via __all__
There was a problem hiding this comment.
👍 Gotcha. Hm, that's odd. It does get used, but then it gets removed via del later on (I don't want it included in the iota namespace).
Eh, maybe the # noqa here is fine.
| from iota import TransactionHash, TransactionTrytes, TryteString # noqa | ||
| from iota import TrytesCompatible # noqa | ||
| from iota.adapter import BaseAdapter, resolve_adapter # noqa | ||
| from iota.commands import BaseCommand, CustomCommand, core # noqa |
There was a problem hiding this comment.
❔ Do these imports need # noqa?
iota/api.py
Outdated
| trytes, | ||
| min_weight_magnitude=None | ||
| ): | ||
| # type: (TransactionHash, TransactionHash, Iterable[TryteString], int) -> dict # noqa |
There was a problem hiding this comment.
❔ Does this guy need a # noqa?
There was a problem hiding this comment.
According to flake8, yes. The line has more than 80 character. I was not sure if these lines are somewhat interpreted by sphinx so I didn't break that line into two shorter ones. Would that be an option instead?
There was a problem hiding this comment.
👍 Gotcha. Those are type hints, used by some IDEs (e.g., PyCharm) for autocompletion, etc.
💡 It is possible to address the line length issue here, by doing this:
def attach_to_tangle(
self,
trunk_transaction, # type: TransactionHash
branch_transaction, # type: TransactionHash
trytes, # type: Iterable[TryteString]
min_weight_magnitude=None # type: Optional[int]
):
# type: (...) -> dictSee PEP-484 for more info.
There was a problem hiding this comment.
That looks neat! 👍
|
|
||
| References: | ||
| - https://github.com/iotaledger/wiki/blob/master/api-proposal.md#getbundle | ||
| """ # noqa |
There was a problem hiding this comment.
❔ Do these need # noqa?
There was a problem hiding this comment.
Flake8 is really picky, I have to admint. line 535 has more than 80 chars... I think the picky-ness of flake can be tuned. I'll checkout if doc strings can be ignored, if # noqa is undesireable.
There was a problem hiding this comment.
👍 Hm. I guess that's fine. think it's useful to check line length on docstrings in general; if flake8 doesn't want to make exceptions for URLs, then we can live with this.
Anyway, the only alternatives I can think of are pretty awful, comparatively (split the URL into multiple lines, use URL shortener).
There was a problem hiding this comment.
Yeah, I also think URLs should be kept the way they are.
I'm going to update this PR asap
| # type: (AdapterSpec, bool) -> None | ||
| """ | ||
| :param adapter: | ||
| URI string or BaseAdapter instance. |
There was a problem hiding this comment.
❔ To be consistent, should we also use 4-space indents inside docstrings?
There was a problem hiding this comment.
I don't know. Sphinx doesn't respect formatting here anyway, right? Ah, let's make it 4 ok?
|
Going to close this PR in favor of #147 |
The PEP8 adoption could be done in one go and bundled in one pull request for all files. But it seems like it could cause quite some merge conflicts if it's not done in shortest time.
So, this is sort of a basis for discussion if the proposed style is fine (especially using 4 vs 2 spaces) for everybody.
flake8finishes errorless.