SunSpecOrangeButton / pyoblib

Orange Button Python Library
Apache License 2.0
14 stars 19 forks source link

move cli.py to oblib #135

Closed cwhanse closed 5 years ago

cwhanse commented 5 years ago

I think that moving cli.py to oblib is a viable solution to the PYTHONPATH issue

cwhanse commented 5 years ago

I agree with (1), (2), (3) and maybe (4) but I couldn't tell exactly which comment you are referring to. One bite at a time, though, maybe we can first get the test environment to work with the current form of cli.py, then take the conversion of cli.py to a module in stages.

joelebwf commented 5 years ago

The Stackoverflow comment I was referring to was the person who suggested the following code clip (although I think its actually a variation on this clip):

mymodule.py

import argparse import sys

def main(args): parser = argparse.ArgumentParser() parser.add_argument('-a') process(**vars(parser.parse_args(args))) return 0

def process(a=None): pass

if name == "main": sys.exit(main(sys.argv[1:]))

Then you can test normally.

import mock

from mymodule import main

@mock.patch('mymodule.process') def test_main(process): main([]) process.assert_call_once_with(a=None)

@mock.patch('foo.process') def test_main_a(process): main(['-a', '1']) process.assert_call_once_with(a='1')

joelebwf commented 5 years ago

If we could move this out to 1.1.0 that would be great. Then I could write a small program to test a few things (such as is it possible to get the argsparse library to stop exiting on error so that it can become a well behaved API Library). It sounds from Slack that Jessie is on track to take care of the short term tactical issue so this could be added to issues and deferred.

cwhanse commented 5 years ago

For the scope of this PR, how about:

  1. move cli.py from scripts to oblib
  2. unittest for the three cli.py functions that are currently addressed in the new file test_cli.py
  3. minor edits in cli.py, e.g., parser to argparser.

I think the next step is to convert cli.py to a module. That will change the pattern for testing, so I don't think it's worth completing the content of test_cli.py as it stands.

joelebwf commented 5 years ago
  1. If its ok with the people who recommended that it goes to scripts in the first place (that might have been Jessie and Jono, I am not sure) its ok with me. One side note is the code coverage report is going to have negative impact.

  2. I have no idea if pytest will run to completion on a call to exit() which is how CLI fails at this point in time. At a minimum force an exit() in a commit to see if the rest of the test cases complete or if everything fails before doing this.

  3. Would parent_parser() or root_parser() be a better name? That would make it more consistent with the documentation for argparse (https://docs.python.org/3/library/argparse.html) I think. "parser" is the variable name in the first example and they refer to children as subparsers, later examples introduce parent parser.

On a final note convert all the functions to private (add leading _) so they don't show up in Readthedocs and add a "if main" statement so the code does not run when imported unless its called by cli.sh.

Lets chat tomorrow.

joelebwf commented 5 years ago

I think this should be a 1.1.0 change such that it can be performed correctly. I can add a issue.

cwhanse commented 5 years ago

Agree.