Pep8 #147

HerrMuellerluedenscheid · 2018-01-24T13:55:44Z

Continuing #146

HerrMuellerluedenscheid · 2018-01-24T13:59:28Z

iota/api.py


-from typing import Dict, Iterable, Optional, Text


I think we have to add these again as they provide extra functionality for some smart IDEs.

Or we leverage typing all the way through? But honestly I am not that keen on this. With a proper documentation one should know what he is doing.

https://docs.python.org/3/library/typing.html

Type hints are amazing 😺

IDEs use them for autocompletion and inspections — they're a great way to boost developer efficiency, and to catch typos and other issues that would otherwise go undetected.

I agree that documentation is also important, but they serve different purposes.

Hi @todofixthis,
which IDE are you using?
Hope this syntactic excess of ours is not too intrusive. Once this is finished and in dev, we tackle the C pearl_diver.

I use PyCharm, but this is actually a standard practise (PEP-484), so I would be surprised if the major IDEs don't support this.

The syntax is a bit verbose, because we are supporting Python 2 and Python 3. If we were only supporting Python 3, we could integrate the type hints directly into the method signature.

Could we put these back in, so that IDE type hints work correctly again?

scottbelden · 2018-01-26T23:57:33Z

I personally think 4 spaces is better, but making the change now is going to completely destroy the history. Also, because the change is being made with other changes it's extremely hard to see what all is being changed. If the 4 space change is going to be applied, it might be cleaner to do that specific change in it's own PR for readability sake.

rpitonak · 2018-02-01T20:58:58Z

Hey, in #151 I changed iota/transaction/creation.py to conforms with PEP8. Can you take a look at it and check if everything is in order, so we can merge? Thank you.
@HerrMuellerluedenscheid @miili

miili · 2018-02-02T09:15:45Z

@todofixthis which PR are we pursuing, this one or #148? We haven't put any more work in, since it seemed obsolete. Yet I disagree with 120 characters per line as allowed by #148.

todofixthis · 2018-02-15T19:13:28Z

Hey guys. Is the PR at a point where we could merge it, even if it's not "complete" yet? Maybe we can merge what we've got so far, and then take a progressive approach with the remaining files.

miili · 2018-02-16T11:00:37Z

Yessir! We can merge what is done so far.
We will continue with leftovers on this PR, if you leave if open.

todofixthis · 2018-02-17T23:40:18Z

Okiedoke! I'll do a quick once-over to make sure everything looks OK, and if all's well, I'll go ahead and merge it.

todofixthis

Thanks for working on this! I've requested some changes, and then let's get this merged, so that we can be on our way to PEP-8 compliance!

todofixthis · 2018-02-17T23:42:52Z

iota/adapter/__init__.py

+            elif response.status_code == codes['internal_server_error']:
+                error = decoded['exception']
+        except KeyError as e:
+            # TODO: shouldn't pass silently


This is intentional, actually; if the response from the node is so badly malformed that we can't extract the error message, then we just assume that the entire response is an error message (see line 444 below).

Maybe we should add a comment to that effect here, so that other developers aren't confused.

todofixthis · 2018-02-17T23:45:02Z

iota/api.py


-from typing import Dict, Iterable, Optional, Text


Could we put these back in, so that IDE type hints work correctly again?

todofixthis · 2018-02-17T23:48:38Z

iota/bin/__init__.py


 import sys
 from abc import ABCMeta, abstractmethod as abstract_method
 from argparse import ArgumentParser
 from getpass import getpass as secure_input
-from io import StringIO


Could we put this back in, so that IDE type hints work correctly?

todofixthis · 2018-02-17T23:50:27Z

iota/bin/repl.py


-from argparse import ArgumentParser


Could we put this back in, so that type hints work correctly?

todofixthis · 2018-02-17T23:51:37Z

iota/commands/__init__.py


 import filters as f
 from six import string_types, with_metaclass

-from iota.adapter import BaseAdapter


Could we put this back in, so that type hints work correctly?

todofixthis · 2018-02-18T00:10:43Z

iota/types.py


 from codecs import decode, encode
 from itertools import chain
 from math import ceil
 from random import SystemRandom
-from typing import Any, AnyStr, Generator, Iterable, Iterator, List, \
-  MutableSequence, Optional, Text, Type, TypeVar, Union


Could we put this back in, so that type hints work correctly?

todofixthis · 2018-02-18T00:12:12Z

iota/types.py

+                   codec=AsciiTrytesCodec.name,  # Text
+                   *args,  # *Any
+                   **kwargs  # **Any
+                   ):  # -> T


Type hints should start with # type: , and the final one should take the form # type: (...) -> T (see PEP-484 for more info).

todofixthis · 2018-02-18T00:15:14Z

iota/types.py

+
+        if isinstance(trytes, TryteString):
+            if type(trytes) is TryteString or issubclass(
+              type(trytes), type(self)):


Could we restore incoming_type? It might be unnecessary technically, but we don't need to refactor it for PEP-8 compliance.

This is already a huge PR that touches virtually every file in the codebase; I'd prefer it if we limited the scope of functional changes as much as possible.

todofixthis · 2018-02-18T00:17:11Z

iota/types.py

+                 balance=None,  # Optional[int]
+                 key_index=None,  # Optional[int]
+                 security_level=None
+                 ):  # -> None


Type hints need fixing.

todofixthis · 2018-02-18T00:17:36Z

iota/types.py

+        super(Address, self).__init__(trytes, pad=self.LEN)
+
+        self.checksum = None
+        # type: Optional[AddressChecksum]


Type hints have to be on the same line as the symbol they are documenting.

HerrMuellerluedenscheid · 2018-02-20T11:32:31Z

Hey @todofixthis thanks for the thorough review! We will finish this until the end of the week. We are a little busy due to a hackathon this week. Taking chances to promote IOTA and this lib a little 😃

miili · 2018-02-20T16:40:36Z

Hi Phonix, thanks for the thorough review. However I would rather promote IDE agnostic code. Less is more!
Particularly libraries which realize standards and are hopefully mentioned for broader adoption.

Moreover Pycharm can extract type hints from docstrings:

https://www.jetbrains.com/help/pycharm/using-docstrings-to-specify-types.html

todofixthis · 2018-02-20T20:05:13Z

Thanks guys! Really appreciate this.

WRT type hints, I'm a bit confused because I thought we were following a Python standard — PEP-484. Is the issue here that other IDEs don't understand those type hints?

The docstring-style type hints are supported in PyCharm, but it's labelled as a legacy feature.

If it turns out that most IDEs do not support PEP-484 but do support docstring type hints (and I really do want to verify that this is the case first), then I agree we should change the code to be more compatible (and no, don't worry; I won't insist on having BOTH docstring and PEP-484 type hints 😹) — but if we go down this route, I'd prefer it if we do that in a separate PR, just to limit the scope of changes.

scottbelden · 2018-02-20T20:38:35Z

I would like to add a vote for PEP484 type hints (not docstring hints). I would also recommend using something like mypy to have the type hint checks as part of the test suite rather than relying on IDEs because 1) not everyone uses an IDE 2) even if they use an IDE, it might not have hint support and 3) the IDE doesn't enforce it like a failure in the CI would.

Typically I like to have a "test" script that invokes flake8, mypy, the actual tests, etc and any failures in any of those would cause a CI failure. Basically something like this:

#!/bin/bash
set -e

echo "running flake8"
flake8 PROJECT

echo "running mypy"
mypy --ignore-missing-imports --strict PROJECT

echo "running pytest"
python -m coverage run --source PROJECT -m pytest -v $@

python -m coverage report -m

This has treated me well and catches problems faster than relying on users to have the right IDE set up.

todofixthis · 2018-02-21T19:36:48Z

I ❤️ the idea of adding style/standards checks to the build+test process. I added one a little while ago to check for problems in the setuptools project metadata, and I'd be totes keen to expand on that.

That said, I think we should put those enhancements into a separate PR, to keep this one focused.

todofixthis · 2018-06-11T20:36:04Z

Hey guys. Really appreciate your hard work on this! This branch has drifted quite a bit, so I'd like to close this PR and propose a new course of action.

I've also started working on progressively migrating everything over to PEP-8; I should have that wrapped up by the end of the week. Once that is wrapped up, let's switch focus to adding flake8 (and maybe mypy?) checks to .travis.yml and cleaning up any outstanding formatting issues, and then we can finally close #145 😺

I hate to discard all the hard work you've put into reformatting the code for PEP-8, but I think that the sheer scope of that undertaking was making the process take a lot longer than anybody wanted it to. So, I'll tackle the boring stuff (reformatting the code), and if you're up for it, you can take on the interesting stuff (setting up automatic checks during the build process and addressing tricky warnings).

Marius Kriegerowski and others added 7 commits January 21, 2018 22:35

api: PEP8

10cb93f

PEP8

c68148d

reformat long line IDE hints

8e21f01

identation

af8a580

wip

a151359

wip

e6c8584

Merge branch 'pep8' of github.com:2square-io/iota.lib.py into pep8

95f1bc0

HerrMuellerluedenscheid mentioned this pull request Jan 24, 2018

PEP8 (WIP) #146

Closed

HerrMuellerluedenscheid commented Jan 24, 2018

View reviewed changes

Marius Kriegerowski and others added 8 commits January 25, 2018 22:57

PEP8

6c39247

bin: pep8

89d3511

commands: pep8

ee05ddc

Merge branch 'develop' into pep8

3e021c7

commands: upd pep8

4bb719c

pep8

fb90313

pep8

fd22557

pep8 wip

1fea5e4

todofixthis mentioned this pull request Jan 26, 2018

Added a check tox env that check-manifest, flake8 and isort #148

Closed

todofixthis mentioned this pull request Feb 1, 2018

ProposedTransaction.__init__ now convert tag value to a Tag object #151

Merged

todofixthis suggested changes Feb 18, 2018

View reviewed changes

Merge branch 'develop' into pep8

fef8da1

todofixthis closed this Jun 11, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pep8 #147

Pep8 #147

HerrMuellerluedenscheid commented Jan 24, 2018

HerrMuellerluedenscheid Jan 24, 2018

miili Jan 24, 2018

todofixthis Jan 24, 2018 •

edited

Loading

miili Jan 26, 2018

todofixthis Jan 26, 2018 •

edited

Loading

todofixthis Feb 17, 2018 •

edited

Loading

scottbelden commented Jan 26, 2018

rpitonak commented Feb 1, 2018

miili commented Feb 2, 2018

todofixthis commented Feb 15, 2018

miili commented Feb 16, 2018

todofixthis commented Feb 17, 2018

todofixthis left a comment

todofixthis Feb 17, 2018

todofixthis Feb 17, 2018 •

edited

Loading

todofixthis Feb 17, 2018

todofixthis Feb 17, 2018

todofixthis Feb 17, 2018

todofixthis Feb 18, 2018

todofixthis Feb 18, 2018

todofixthis Feb 18, 2018

todofixthis Feb 18, 2018

todofixthis Feb 18, 2018

HerrMuellerluedenscheid commented Feb 20, 2018

miili commented Feb 20, 2018

todofixthis commented Feb 20, 2018 •

edited

Loading

scottbelden commented Feb 20, 2018 •

edited

Loading

todofixthis commented Feb 21, 2018 •

edited

Loading

todofixthis commented Jun 11, 2018

Pep8 #147

Pep8 #147

Conversation

HerrMuellerluedenscheid commented Jan 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

todofixthis Jan 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

todofixthis Jan 26, 2018 • edited Loading

Choose a reason for hiding this comment

todofixthis Feb 17, 2018 • edited Loading

Choose a reason for hiding this comment

scottbelden commented Jan 26, 2018

rpitonak commented Feb 1, 2018

miili commented Feb 2, 2018

todofixthis commented Feb 15, 2018

miili commented Feb 16, 2018

todofixthis commented Feb 17, 2018

todofixthis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

todofixthis Feb 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HerrMuellerluedenscheid commented Feb 20, 2018

miili commented Feb 20, 2018

todofixthis commented Feb 20, 2018 • edited Loading

scottbelden commented Feb 20, 2018 • edited Loading

todofixthis commented Feb 21, 2018 • edited Loading

todofixthis commented Jun 11, 2018

todofixthis Jan 24, 2018 •

edited

Loading

todofixthis Jan 26, 2018 •

edited

Loading

todofixthis Feb 17, 2018 •

edited

Loading

todofixthis Feb 17, 2018 •

edited

Loading

todofixthis commented Feb 20, 2018 •

edited

Loading

scottbelden commented Feb 20, 2018 •

edited

Loading

todofixthis commented Feb 21, 2018 •

edited

Loading