Skip to content
This repository has been archived by the owner on Jan 13, 2023. It is now read-only.

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

Closed
wants to merge 1 commit into from

Conversation

euri10
Copy link

@euri10 euri10 commented Jan 26, 2018

(the latest not activated yet, tbc)
Added missing files to MANIFEST.in
Configured flake8 in setup.cfg to authorize 120 chars line-length, how
gentle !
Fixed numerous non-PEP8 compliant spots, in iota / test and examples
folder (ugly commenting of numerous useless imports to make sure it
doesn't break anything, which doesn't seem to be the case so far)

just realize I may have better done that from dev branch....

not activated yet, tbc)
Added missing files to MANIFEST.in
Configured flake8 in setup.cfg to authorize 120 chars line-length, how
gentle !
Fixed numerous non-PEP8 compliant spots, in iota / test and examples
folder (ugly commenting of numerous useless imports to make sure it
doesn;t break anything, which doesn't seem to be the case so far)
@todofixthis todofixthis changed the base branch from master to develop January 26, 2018 21:48
@todofixthis
Copy link
Contributor

Hey @euri10. Thanks for your pull request!!

I really like this idea, especially fixing the metadata and automating the checks!

I went ahead and changed the target branch to develop. Unfortunately, that caused a bunch of conflicts to appear.

Have a look at #147 — maybe you can collaborate with @HerrMuellerluedenscheid and @miili .

@euri10
Copy link
Author

euri10 commented Jan 26, 2018 via email

@HerrMuellerluedenscheid
Copy link
Contributor

Hey @euri10, have a look also at #146 where we discussed already a few points regarding pep8 details. We'd.be glad to have you on board to finish the pep8 stylings soon :)

@todofixthis
Copy link
Contributor

chur chur

Imports

If the import appears in an __init__.py, then it's probably just an attempt to flatten the namespace a bit.

E.g., it's convenient for developers to do from iota import TryteString rather than from iota.types import TryteString, etc.

There's also an import in iota/crypto/__init__.py that tries to import the Curl class from the ccurl extension, but if the extension is not installed, it will fall back to the pure-Python (slower) implementation.

If there are any other imports that fail the check, let me know, and I can probably provide more context.

Docstrings

The docstrings should use reStructuredText (some IDEs, such as PyCharm, know how to render them). reStructuredText traditionally uses 3-space indents, but it will work just fine with 2-si, 4-si, etc. Feel free to re-indent those as well (:

Do I work for Google?

jajaja If only, if only. I just like the horizontally-compressed look; it requires my eyes to do less horizontal scanning to read the code (or at least, I think it does; I haven't exactly measured...).

But, PEP-8 (as well as a number of PyOTA community members!) says 4-space indents, so I'm prefectly happy to switch.

@HerrMuellerluedenscheid
Copy link
Contributor

Just had a closer look at you pr. Are you done with all modules already in terms of pep8?

@euri10
Copy link
Author

euri10 commented Jan 26, 2018 via email

@miili
Copy link

miili commented Jan 26, 2018

Please start watching this repo.

@euri10
Copy link
Author

euri10 commented Jan 28, 2018

I put the changes to develop on my fork and dunno if it's possible to keep same PR now that it's on another branch also on my side :)

Added docs checking too and cleant some old links along the way now that linkcheck is enabled.

Read #147 and realize I commented most of the typing import because they're unused and flake8 complained, there's probably a way to keep them without adding for each and every one a noqa:f401

@miili
Copy link

miili commented Jan 28, 2018

Edit PR, change base.

@miili miili mentioned this pull request Feb 2, 2018
@todofixthis
Copy link
Contributor

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).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants