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

multi feature request: Python 3 support + style + logging + documentation #7

Closed
neitsa opened this issue Oct 5, 2016 · 8 comments
Closed

Comments

@neitsa
Copy link

neitsa commented Oct 5, 2016

Hello @ajventer, thank you for your great work :)

I have a couple of requests, they are all optional:

  • Consider using a Linter for your source (I'd recommend flake8)
  • Consider using the logging module rather than a DEBUG flag
  • Consider supporting python 3 (possibly using the six module (available as a standard module on most platforms), and/or using __future__ for basic stuffs)
  • Consider documenting at all levels (package, module, class, function) using a known doc style (Plain, Epytext, restructuredText, Numpy, Google, etc.)
  • [very optional] Consider using type hints (a.k.a PEP 484) or even the typing module (see also PEP 526) if you switch to python3.

That may sound like a lot of nitpicking (notably about the style), but that would make the source code little bit more readable and would probably find some possible defects. Flake8 is by default configured for a hard line break at 80 chars, but you might want to extend that a little bit (e.g. 100).

Thank you o/

@ajventer
Copy link
Owner

ajventer commented Oct 5, 2016

Consider using a Linter for your source (I'd recommend flake8)
Yes. Was working to get a decent public beta out as fast a possible so I skipped this step but I'm a fan of linter's myself and it will be happening before I start on the GUI version.

Consider using the logging module rather than a DEBUG flag
I'll think about it, my one concern with the logging module is that it requires more complex commandline stuff to really tell it what you want to do. As it is I'm a bit unhappy with the structure that argparse led me to - I would love to simply the parameters further. But it's probably the right way to go.

Consider supporting python 3 (possibly using the six module (available as a standard module on most platforms), and/or using future for basic stuffs)
Already on the TODO list :) I'll look into using SIX - was planning to just use future in theory the only thing I use which is significantly affected is having to import the print statement and list-cast the stuff like .keys() which are generators in 3.

Consider documenting at all levels (package, module, class, function) using a known doc style (Plain, Epytext, restructuredText, Numpy, Google, etc.)
A very good point, goes right up with my already intended plan to expand the doctests significantly - even to things which really don't lend well to testing that way if only to force the test-runner to syntax check everything.

[very optional] Consider using type hints (a.k.a PEP 484) or even the typing module (see also PEP 526) if you switch to python3.
I'll look into it. I'll add these all to the official TODO list. If you feel like making a start on any of them - I'll accept related pull requests on the master branch but until the beta is stable I'm not doing anything except bugfixes myself.

@neitsa
Copy link
Author

neitsa commented Oct 5, 2016

Thank you for your answer!

I'd like to do a simple linter pass first (that is, not changing anything related to the code logic). What would be your preference for max line length? (I usually stick with the 80 chars hard line break, but that's a matter of personal preference).

@ajventer
Copy link
Owner

ajventer commented Oct 5, 2016

Line length is the one linter warning I usually ignore - simply because the syntax for line-splits has never been very obvious for me, or very consistent.
I would merge a pure linter update - just test it to rule out typos and syntax errors (though the linter should itself catch most of those).
So since I don't have a strong opinion myself, let's go with 80 chars which is the closest thing to a standard out there (even though 80-character terminals are virtually extinct).

@SYZYGY-DEV333
Copy link
Contributor

I have added py3 support.

@ajventer
Copy link
Owner

ajventer commented Oct 8, 2016

I reviewed the patch, and have merged it to the master branch.

@ajventer
Copy link
Owner

@neitsa I am preparing for a 0.1.0 stable release sometime next week. At this stage there are no more critical bugs I know about. I would like to include your linter patch in that if you can have it ready in time. If you can get it to me by Wednesday or so, I'll have time to test and review and we can release by next weekend. Otherwise linting has to wait for the next release.

Your other feature requests I'm pushing back to 0.2.0 though they will be part of the major focus in the upcoming release. My biggest concern this time round was just to get the thing stable which, considering the enormity of variation in modules and the very bizarre ways CKAN represents them was a major job. I now have the most modded KSP game I have ever had as I tried to throw the most complex mods I could find at the thing to see what breaks.

@neitsa
Copy link
Author

neitsa commented Oct 15, 2016

Awesome @ajventer ! 😃 I started to make Flake8 passes on the code but haven't finished yet :(. I don't know if it will be ready next week (we are gonna release RemoteTech), but I'll try to do my best. I don't want to push a half finished job though.

Anyway, thanks a lot for your feedback and your project! Keep up the good work 👍

@ajventer
Copy link
Owner

Good to hear RT is under active development still :) will be interesting to see how you guys integrate it with commnet. It's not critical -a linter pass is ultimately just about readability, not operation and while that's important it's not critical. If we can't get it in, then we do it for the next release, I just wanted to give you an opportunity to get your work in if you want to. I'll probably not release before Saturday so let's see what happens.

@ajventer ajventer closed this as completed May 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants