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

Make discovery to work with no IP addr and token, courtesy of M0ses #198

Merged
merged 4 commits into from
Jan 30, 2018

Conversation

rytilahti
Copy link
Owner

Obsoletes #194

@@ -25,6 +26,7 @@ def validate_ip(ctx, param, value):


def validate_token(ctx, param, value):
if value is None: return None

Choose a reason for hiding this comment

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

multiple statements on one line (colon)

@@ -17,6 +17,7 @@


def validate_ip(ctx, param, value):
if value is None: return None

Choose a reason for hiding this comment

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

multiple statements on one line (colon)

@coveralls
Copy link

coveralls commented Jan 30, 2018

Coverage Status

Coverage increased (+0.4%) to 67.761% when pulling 4d792b5 on allow_discovery_without_ip_token into e90b7e3 on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.04%) to 67.356% when pulling 842d895 on allow_discovery_without_ip_token into e90b7e3 on master.

@M0ses
Copy link

M0ses commented Jan 30, 2018

LGTM, but maybe adding my test cases in an adapted version would make sense and could avoid regressions


def test_validate_ip_empty():
assert validate_ip(None, None, None) is None

Choose a reason for hiding this comment

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

blank line at end of file

assert not validate_token(None, None, None)

def test_validate_ip_empty():
assert validate_ip(None, None, None) is None

Choose a reason for hiding this comment

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

'validate_ip' may be undefined, or defined from star imports: miio.click_common

def test_validate_token_empty():
assert not validate_token(None, None, None)

def test_validate_ip_empty():

Choose a reason for hiding this comment

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

expected 2 blank lines, found 1

from miio.click_common import *

def test_validate_token_empty():
assert not validate_token(None, None, None)

Choose a reason for hiding this comment

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

'validate_token' may be undefined, or defined from star imports: miio.click_common

@@ -0,0 +1,8 @@
from miio.click_common import *

def test_validate_token_empty():

Choose a reason for hiding this comment

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

expected 2 blank lines, found 1

@@ -0,0 +1,8 @@
from miio.click_common import *

Choose a reason for hiding this comment

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

'from miio.click_common import *' used; unable to detect undefined names


def test_validate_ip_empty():
assert validate_ip(None, None, None) is None

Choose a reason for hiding this comment

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

blank line at end of file

assert not validate_token(None, None, None)

def test_validate_ip_empty():
assert validate_ip(None, None, None) is None

Choose a reason for hiding this comment

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

'validate_ip' may be undefined, or defined from star imports: miio.click_common

def test_validate_token_empty():
assert not validate_token(None, None, None)

def test_validate_ip_empty():

Choose a reason for hiding this comment

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

expected 2 blank lines, found 1

from miio.click_common import *

def test_validate_token_empty():
assert not validate_token(None, None, None)

Choose a reason for hiding this comment

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

'validate_token' may be undefined, or defined from star imports: miio.click_common

@@ -0,0 +1,8 @@
from miio.click_common import *

def test_validate_token_empty():

Choose a reason for hiding this comment

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

expected 2 blank lines, found 1

@@ -0,0 +1,8 @@
from miio.click_common import *

Choose a reason for hiding this comment

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

'from miio.click_common import *' used; unable to detect undefined names

@rytilahti
Copy link
Owner Author

Good point! I renamed the file (hopefully there'll be more tests for that part of the code) and changed test to assert with 'is' instead of '=='. Thanks for the patch!


def test_validate_ip_empty():
assert validate_ip(None, None, None) is None

Choose a reason for hiding this comment

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

blank line at end of file

@rytilahti rytilahti changed the title Make discovery to work with no IP addr and token Make discovery to work with no IP addr and token, courtesy of M0ses Jan 30, 2018
@rytilahti rytilahti merged commit 93b8e0f into master Jan 30, 2018
@rytilahti rytilahti deleted the allow_discovery_without_ip_token branch January 30, 2018 21:41
@M0ses
Copy link

M0ses commented Jan 30, 2018

Thanks for fixing.

Just some nitpicking: Wouldn`t it make sense to change https://github.com/rytilahti/python-miio/pull/198/files#diff-57c62bdfa1f30df2fde08081703b47c5R5 to

assert validate_token(None, None, None) is None

as done below in https://github.com/rytilahti/python-miio/pull/198/files#diff-57c62bdfa1f30df2fde08081703b47c5R9 to be more consistent in coding?

BTW: I built rpm packages for openSUSE https://build.opensuse.org/project/show/home:M0ses:miio for those who want to try it on tumbleweed

@rytilahti
Copy link
Owner Author

Yeah, I think it would have made sense to compare both to is None, considering that "not" doesn't compare to Noneness but to trueness/falseness. That can probably be fixed at some point when new tests get added to that suite, I think it's not worth to change it for now.

Thanks for the packaging! If you want, the README could be updated to point to available packaging, I think archlinux has also a package available :-)

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.

4 participants