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

Use docopt to handle example tests arguments. #287

Merged

Conversation

come-maiz
Copy link
Contributor

LP: #1533395

@kyrofa
Copy link
Contributor

kyrofa commented Feb 3, 2016

Looks good to me 👍 .

if arguments['--port']:
tests.config['port'] = arguments['--port']
if arguments['--filter']:
tests.config['filter'] = arguments['--filter']
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a problem down under if we just do this:

test.config['ip'] = arguments['--ip']
tests.config['port'] = arguments['--port']
tests.config['filter'] = arguments['--filter']

then for test.config you don't need to check for the existence of the key but the key directly i.e. if test.config['filter']

Does this make sense, I have plenty of assumptions in this comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only the difference between having a dictionary element with None as a value and not having it.

>>> {}.get('port', 22)
22
>>> {'port': None}.get('port', 22)
>>>

I'll make the change to see if you like it better. I slightly prefer to put the complexity here than on the test; but on the other hand it sounds better to handle '', None and unexistin

@sergiusens
Copy link
Collaborator

Looks good, but what is test or why is it still running?

@come-maiz
Copy link
Contributor Author

test is not running, it was just a test that I could communicate to this PR from jenkins. Now I don't know how to delete it :)

@come-maiz come-maiz force-pushed the wishlist/1533395/docopt_tests branch from 96c2c8f to 51c66c4 Compare February 5, 2016 14:34
@@ -214,14 +214,17 @@ def setUpClass(cls):
cls.temp_dir = tempfile.mkdtemp()
if not config.get('skip-install', False):
ip = config.get('ip', None)
if ip is None:
if not ip:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this even if not relevant 😄

@come-maiz come-maiz force-pushed the wishlist/1533395/docopt_tests branch from 51c66c4 to 8eb9150 Compare February 5, 2016 18:33
@sergiusens
Copy link
Collaborator

So 👍 then

sergiusens added a commit that referenced this pull request Feb 5, 2016
Use docopt to handle example tests arguments.
@sergiusens sergiusens merged commit 9e78357 into canonical:master Feb 5, 2016
smoser pushed a commit to smoser/snapcraft that referenced this pull request Sep 14, 2016
@come-maiz come-maiz deleted the wishlist/1533395/docopt_tests branch January 30, 2017 02:57
kalikiana pushed a commit to kalikiana/snapcraft that referenced this pull request Apr 6, 2017
…tests

Use docopt to handle example tests arguments.
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

Successfully merging this pull request may close these issues.

None yet

3 participants