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

Return None value during style validation, before running into exception #4

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@stantonxu
Copy link

stantonxu commented Jul 17, 2018

This is fixing #3

If the style value equals to 'none', set it to None and return it, before the validate function running into exception.

@freakboy3742 , could you please review?

@stantonxu

This comment has been minimized.

Copy link
Author

stantonxu commented Jul 17, 2018

@freakboy3742 , it looks like the smoke test is expecting an exception when the value is set to 'none'. I don't quite understand yet. Why?

@freakboy3742

This comment has been minimized.

Copy link
Member

freakboy3742 commented Jul 17, 2018

@stantonxu From the look of it, you’ve made a change that addresses the problem @mcrowson has reported, but it breaks other behavior. In particular, any property that has a type of Color, Integer or Number isn’t allowed to have a value of 'none', so there’s a specific test of that behavior in the test suite - and those tests are failing, because your logic returns immediately if a value of 'none' is found.

@@ -50,6 +50,7 @@ def validate(self, value):
pass
if value == 'none':
value = None
return value

This comment has been minimized.

@rayrrr

rayrrr Jul 17, 2018

Contributor

Instead of doing return value here, try removing:

        if value == 'none':
             value = None

See if that gets the tests to pass.

This comment has been minimized.

@stantonxu

stantonxu Jul 17, 2018

Author

@rayrrr , thanks. I've already tried that, doesn't work.

When I comment out the two lines,

        if value == 'none':
             value = None

The test cases of 'test_multiple_choices', 'test_none', 'test_values' failed.

This comment has been minimized.

@rayrrr

rayrrr Jul 17, 2018

Contributor

Too bad. I'll give it some more thought too and see if I can help you get this done.

This comment has been minimized.

@stantonxu

stantonxu Jul 17, 2018

Author

Thank you @rayrrr .

This comment has been minimized.

@rayrrr

rayrrr Jul 18, 2018

Contributor

@stantonxu you should be able to fix this without changing any existing tests by removing the return value line, and changing the line if value == 'none': to (pseudocode) if value == 'none' and <type is Color, Integer, or Number>:. Try that.

This comment has been minimized.

@stantonxu

stantonxu Jul 18, 2018

Author

The validate code is as below, so the <type is Color, Integer, or Number>: code won't hit.

    def validate(self, value):
        if self.default:
            if value is None:
                return None
        if self.string:
            try:
                return value.strip()
            except AttributeError:
                pass
        if self.integer:
            try:
                return int(value)
            except (ValueError, TypeError):
                pass
        if self.number:
            try:
                return float(value)
            except (ValueError, TypeError):
                pass
        if self.color:
            try:
                return color(value)
            except ValueError:
                pass
        if value == 'none':
            value = None
            return value
        for const in self.constants:
            if value == const:
                return const

        raise ValueError("'{0}' is not a valid initial value".format(value))

This comment has been minimized.

@rayrrr

rayrrr Jul 18, 2018

Contributor

What we want to do is change 'none' to None for Color, Integer, and Number types. Let's do that before the conditional checks for those types.

 def validate(self, value):
        if self.default:
            if value is None:
                return None
        if value == 'none' and (self.color or self.integer or self.number):
            value = None
        if self.string:
            try:
                return value.strip()
            except AttributeError:
                pass
        if self.integer:
            try:
                return int(value)
            except (ValueError, TypeError):
                pass
        if self.number:
            try:
                return float(value)
            except (ValueError, TypeError):
                pass
        if self.color:
            try:
                return color(value)
            except ValueError:
                pass
        for const in self.constants:
            if value == const:
                return const

        raise ValueError("'{0}' is not a valid initial value".format(value))

This comment has been minimized.

@rayrrr

rayrrr Jul 18, 2018

Contributor

@stantonxu once #5 and pybee/toga#564 are merged, the last
code block above might work. What do you think?

This comment has been minimized.

@rayrrr

rayrrr Jul 18, 2018

Contributor

Actually, the other PR's might not be needed, but I still think they'll make these apps more tidy. Try the code block as is and see what happens.

This comment has been minimized.

@stantonxu

stantonxu Jul 18, 2018

Author

@rayrrr thanks for the suggestion, however it doesn't work out. Here is the error from test,

======================================================================
ERROR: test_none (tests.test_choices.PropertyChoiceTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/xus12/git/pybee/travertino/travertino/declaration.py", line 181, in setter
    value = choices.validate(value)
  File "/Users/xus12/git/pybee/travertino/travertino/declaration.py", line 60, in validate
    raise ValueError("'{0}' is not a valid initial value".format(value))
ValueError: 'none' is not a valid initial value

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/Users/xus12/git/pybee/travertino/tests/test_choices.py", line 46, in test_none
    obj.prop = 'none'
  File "/Users/xus12/git/pybee/travertino/travertino/declaration.py", line 184, in setter
    value, name, choices
ValueError: Invalid value 'none' for property 'prop'; Valid values are: none

======================================================================
ERROR: test_values (tests.test_choices.PropertyChoiceTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/xus12/git/pybee/travertino/travertino/declaration.py", line 181, in setter
    value = choices.validate(value)
  File "/Users/xus12/git/pybee/travertino/travertino/declaration.py", line 60, in validate
    raise ValueError("'{0}' is not a valid initial value".format(value))
ValueError: 'none' is not a valid initial value

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/Users/xus12/git/pybee/travertino/tests/test_choices.py", line 260, in test_values
    obj.prop = 'none'
  File "/Users/xus12/git/pybee/travertino/travertino/declaration.py", line 184, in setter
    value, name, choices
ValueError: Invalid value 'none' for property 'prop'; Valid values are: a, b, none

----------------------------------------------------------------------
Ran 84 tests in 0.038s

FAILED (errors=2)
@rayrrr

This comment has been minimized.

Copy link
Contributor

rayrrr commented Jul 18, 2018

@freakboy3742 I'm pretty sure we need to change the unit tests here, based on the visibility choices for a Toga Box object: visible, hidden, and none.

Right now the tests assume every value of none here will be changed to a Python None object, but we only want to do that for certain types (color, integer, and number). Can we change the tests accordingly, to remove the tests that check that the string none gets changed to a None object in all cases, since it no longer will?

@stantonxu

This comment has been minimized.

Copy link
Author

stantonxu commented Jul 20, 2018

@freakboy3742 , do you have any comments? I think we probably don't need to update the test cases, but am struggling where the problem is with the code which prevent the testing from successful.

@freakboy3742

This comment has been minimized.

Copy link
Member

freakboy3742 commented Jul 21, 2018

@stantonxu @rayrrr I don't think the tests need to be altered (or I'm misunderstanding which test you're suggesting needs to be changed). The test_allow_color test, for example, validates that a property whose choices allow for Color options don't allow anything else. The test validates (and, int this patch, fails) when provided 'none' as a value - which is the correct behavior.

@stantonxu

This comment has been minimized.

Copy link
Author

stantonxu commented Jul 22, 2018

@freakboy3742 , I agree with you, I think the test cases are valid. However, for the Validate method, any suggestion how to make it work?

If it is color, or integer, or string, in the Validate method, it should already return before it runs into the section of code to raise ValueError exception.

@rayrrr

This comment has been minimized.

Copy link
Contributor

rayrrr commented Jul 22, 2018

@freakboy3742 and @stantonxu it was a bit tough to explain what tests I thought should be changed so I made #6 which has code changes along with passing tests and should solve the original Box issue. So if you agree, we can just merge it after code review. Otherwise we can use it as a reference point for further discussion here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment