Added main.py to execute package with `python -m restview` #53

SimplyKnownAsG · 2018-04-28T20:01:59Z

This addresses #52. I'm not sure if it makes sense to keep the same docstring in this file. I can remove it altogether or shorten to simply:

"""
Usage:
    python -m restview --help
"""

given that argparse kinda handles the rest.

Other questions:

do you want to remove the if __name__ == '__main__' from restviewhttp.py?
if yes to the above, should I change the setup.py to use __main__ instead of the restview.restviewhttp:main?
if yes to the above, should we chmod -x restviewhttp.py? (no effect on windows)

coveralls · 2018-04-28T20:06:34Z

Coverage decreased (-0.2%) to 99.782% when pulling 9963650 on SimplyKnownAsG:master into 04d712e on mgedmin:master.

coveralls · 2018-04-28T20:06:34Z

Coverage decreased (-0.2%) to 99.782% when pulling 9963650 on SimplyKnownAsG:master into 04d712e on mgedmin:master.

coveralls · 2018-04-28T20:06:34Z

Coverage decreased (-0.2%) to 99.782% when pulling 9963650 on SimplyKnownAsG:master into 04d712e on mgedmin:master.

mgedmin · 2018-04-29T07:46:43Z

A shorter docstring makes more sense than a duplicated docstring.

do you want to remove the if __name__ == '__main__' from restviewhttp.py?

Yes, please. And maybe add it to __main__.py?

if yes to the above, should I change the setup.py to use __main__ instead of the restview.restviewhttp:main?

I don't think that's necessary.

if yes to the above, should we chmod -x restviewhttp.py? (no effect on windows)

Yes, please, and also remove the #! line if it's there. (I've a vim plugin that chmod +x's every file that has a #! line on save.)

mgedmin

Tests fail on Appveyor because __main__.py has no if __name__ == '__main__': check and tries to run main() on import during test discovery. This needs to be fixed.

(I wonder why tests don't break on Travis CI?)

The rest looks good, but it would look even better if more of your suggested changes were made.

Also, can you add a short entry to CHANGES.rst, something about "python -m restview is now supported"?

SimplyKnownAsG · 2018-04-30T17:25:11Z

I may have overdone the changes, feel free to ask to revert them...

I took some input from this page on how to "do Python apps the right way", obviously, that is an opinion...

Also, I didn't realize that __name__ == '__main__' is only true for when the package is executed with -m. As noted in one of the comments in the article, python -c "import restview.__main__" has a __name__ of "restview.__main__".

mgedmin

I would prefer it if main() stayed in restviewhttp.py. I'm not sure why this is; possibly because I don't like to see underscores in import statements (in the test suite), possibly because I learned the lesson of not putting code in __init__.py and now am transferring that lesson to all dunder modules without proper justification.

I'll LGTM this, but leave it open for a couple of days, in case you want to make it flake8-clean etc.

mgedmin · 2018-05-02T07:48:01Z

src/restview/__init__.py

@@ -0,0 +1,3 @@
+
+__version__ = '2.8.2.dev0'
+


flake8 is going to complain about the blank line at the end of the file.

The tool I use to automate releases (zest.releaser) needs to be told which Python file contains the __version__ definition so it can update it. This is specified in setup.cfg, which now needs to be updated.

mgedmin · 2018-05-02T07:49:55Z

src/restview/__main__.py

+
+from restview import __version__
+from restview import restviewhttp
+from restview.restviewhttp import RestViewer


This mixing of import styles feels weird to me. If you're using restviewhttp.<stuff>, why not use restviewhttp.RestViewer as well?

yea, actually that is basically enough of a reason for reverting back to having it all in restviewhttp... It is an unfortunate side-effect of how Mock and patch work. Without using the module-specific methods, patch wouldn't register properly.

mgedmin · 2018-05-02T07:50:48Z

src/restview/restviewhttp.py

@@ -681,118 +661,3 @@ def launch_browser(url):
    t.start()




This leaves multiple blank lines at end of file for flake8 to complain about ;)

mgedmin · 2018-05-02T07:52:51Z

src/restview/tests.py

@@ -954,7 +955,8 @@ def run_main(self, *args, **kw):

    def test_help(self):
        stdout, stderr = self.run_main('--help')
-        self.assertTrue('restview [options] root' in stdout, stdout)
+        self.assertTrue('restview [-h]' in stdout, stdout)
+        self.assertTrue('[root [root ...]]' in stdout, stdout)


Ok, this part makes me unhappy.

You moved code from one module to another and changed it at the same time, which means now I cannot see the changes in the diff.

And I had that custom usage= argument there for a reason (purely aesthetical, but still).

I certainly wasn't trying to slip something by, sorry if that seemed the case.

I'm happy to revert. I thought there was some confusion over the rather detailed custom usage in the restviewhttp.py docstring vs. the custom usage= argument. I spent some time trying to use both the long-form docstring and custom usage=, but it got ugly.... usage='\n'.join(__doc__.split('\n')[2:-3]).

Anyway, I should have realized that this was something you cared about, due to the test.

mgedmin · 2018-05-02T07:54:48Z

src/restview/__main__.py

+
+if __name__ == '__main__':
+    main()
+


flake8 will complain about the blank line,

(I should add flake8 to my Travis config TBH.)

mgedmin · 2018-05-02T07:55:22Z

src/restview/restviewhttp.py

-
-Needs docutils and a web browser. Will syntax-highlight code or doctest blocks
-(needs pygments).
-"""


I would keep a short version of this, just

""" HTTP-based ReStructuredText viewer. """

mgedmin · 2018-05-02T07:58:09Z

src/restview/__main__.py

+from restview.restviewhttp import RestViewer
+
+
+def main():


I don't know why my gut feeling is against having actual code in __main__.py. I'll have to think about it some more (and maybe re-read that blog post you linked, which I'm sure I must've read before).

ok, I don't have a strong preference, I just like restview :-)

mgedmin

Can you add the CHANGES.rst entry crediting yourself with supporting python -m restview?

mgedmin · 2018-05-02T19:48:40Z

src/restview/restviewhttp.py

    parser = argparse.ArgumentParser(
                    usage="%(prog)s [options] root [...]",
                    description="Serve ReStructuredText files over HTTP.",
-                    prog=progname)
+                    prog="restview")


Ooh, what's sys.argv[0] look like when you do python -m restview?

sys.argv[0] is /path/to/restview/src/restview/__main__.py

Haha yeah, good catch!

SimplyKnownAsG

done, hopefully :-)

SimplyKnownAsG · 2018-05-03T03:05:07Z

src/restview/restviewhttp.py

    parser = argparse.ArgumentParser(
                    usage="%(prog)s [options] root [...]",
                    description="Serve ReStructuredText files over HTTP.",
-                    prog=progname)
+                    prog="restview")


sys.argv[0] is /path/to/restview/src/restview/__main__.py

mgedmin · 2018-05-03T07:48:16Z

Thank you!

mgedmin requested changes Apr 29, 2018

View reviewed changes

mgedmin approved these changes May 2, 2018

View reviewed changes

Added __main__.py to execute package with python -m restview

17c28c8

SimplyKnownAsG commented May 3, 2018

View reviewed changes

mgedmin merged commit cfb57e3 into mgedmin:master May 3, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added main.py to execute package with `python -m restview` #53

Added main.py to execute package with `python -m restview` #53

SimplyKnownAsG commented Apr 28, 2018

coveralls commented Apr 28, 2018

coveralls commented Apr 28, 2018

coveralls commented Apr 28, 2018 •

edited

Loading

mgedmin commented Apr 29, 2018

mgedmin left a comment

SimplyKnownAsG commented Apr 30, 2018 •

edited

Loading

mgedmin left a comment

mgedmin May 2, 2018

mgedmin May 2, 2018 •

edited

Loading

mgedmin May 2, 2018

SimplyKnownAsG May 2, 2018

mgedmin May 2, 2018

mgedmin May 2, 2018

SimplyKnownAsG May 2, 2018

mgedmin May 2, 2018

mgedmin May 2, 2018

mgedmin May 2, 2018

SimplyKnownAsG May 2, 2018 •

edited

Loading

mgedmin left a comment

mgedmin May 2, 2018

SimplyKnownAsG May 3, 2018

mgedmin May 3, 2018

SimplyKnownAsG left a comment

SimplyKnownAsG May 3, 2018

mgedmin commented May 3, 2018

Added __main__.py to execute package with python -m restview #53

Added __main__.py to execute package with python -m restview #53

Conversation

SimplyKnownAsG commented Apr 28, 2018

coveralls commented Apr 28, 2018

coveralls commented Apr 28, 2018

coveralls commented Apr 28, 2018 • edited Loading

mgedmin commented Apr 29, 2018

mgedmin left a comment

Choose a reason for hiding this comment

SimplyKnownAsG commented Apr 30, 2018 • edited Loading

mgedmin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgedmin May 2, 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

SimplyKnownAsG May 2, 2018 • edited Loading

Choose a reason for hiding this comment

mgedmin left a 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

SimplyKnownAsG left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgedmin commented May 3, 2018

Added main.py to execute package with `python -m restview` #53

Added main.py to execute package with `python -m restview` #53

coveralls commented Apr 28, 2018 •

edited

Loading

SimplyKnownAsG commented Apr 30, 2018 •

edited

Loading

mgedmin May 2, 2018 •

edited

Loading

SimplyKnownAsG May 2, 2018 •

edited

Loading