Skip to content

Commit

Permalink
[processing] correctly parse default value for boolean parameters upo…
Browse files Browse the repository at this point in the history
…n construction
  • Loading branch information
volaya committed Jan 6, 2016
1 parent bde0f10 commit 946f4e4
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion python/plugins/processing/core/parameters.py
Expand Up @@ -123,7 +123,7 @@ def tr(self, string, context=''):
class ParameterBoolean(Parameter):

def __init__(self, name='', description='', default=None, optional=False):
Parameter.__init__(self, name, description, default, optional)
Parameter.__init__(self, name, description, parseBool(default), optional)

def setValue(self, value):
if value is None:
Expand Down

11 comments on commit 946f4e4

@nyalldawson
Copy link
Collaborator

Choose a reason for hiding this comment

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

@volaya @medspx does the unit test need to be updated to reflect this change or has it introduced a regression? The processing tests are failing since this commit.

@volaya
Copy link
Contributor Author

@volaya volaya commented on 946f4e4 Jan 6, 2016

Choose a reason for hiding this comment

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

Most likely the tests should be adapted. @m-kuhn?

@m-kuhn
Copy link
Member

@m-kuhn m-kuhn commented on 946f4e4 Jan 6, 2016

Choose a reason for hiding this comment

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

Seems that None is no longer accepted as value for boolean parameters, so there is no way to specify an optional parameter.

What's fixed with this commit? Would it make sense to make parseBool aware of None?

The failing code is

    parameter = ParameterBoolean('myName', 'myDescription') # default=None, optional=False
    self.assertEqual(parameter.value, None)

@michaelkirk

@michaelkirk
Copy link
Contributor

Choose a reason for hiding this comment

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

Failure is pretty clear:

Traceback (most recent call last):
  File "/home/travis/build/qgis/QGIS/python/plugins/processing/tests/ParametersTest.py", line 64, in testSetValue
    self.assertEqual(parameter.value, None)
AssertionError: False != None

https://github.com/qgis/QGIS/blob/master/python/plugins/processing/tests/ParametersTest.py#L64

62    def testSetValue(self):
63        parameter = ParameterBoolean('myName', 'myDescription')
64        self.assertEqual(parameter.value, None)

From just looking at the code, I would expect a newly initialized optional BooleanParameter to have a value of None not False. It seems like the test should stay and the application should change.

@m-kuhn
Copy link
Member

@m-kuhn m-kuhn commented on 946f4e4 Jan 6, 2016

Choose a reason for hiding this comment

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

@michaelkirk it seems to me that this test actually produces an non-optional parameter (see my last comment for the defaults)

BUT the defaults are actually conflicting default=None, optional=False AND even if it was optional, None would not be accepted, so the code seems actually to show room for improvement.

@michaelkirk
Copy link
Contributor

Choose a reason for hiding this comment

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

@m-kuhn you're right. Parameters are required by default.

I think that we should maintain a distinction between None and False. So the brain dead way would be something like:

if default != None:
  default = parseBool(default)
Parameter.__init__(self, name, description, default, optional)

@volaya can you give some insight into why you made this change? What is using the ParameterBoolean class that passes (non boolean) defaults that must first be parsed?

edit: code

@m-kuhn
Copy link
Member

@m-kuhn m-kuhn commented on 946f4e4 Jan 6, 2016

Choose a reason for hiding this comment

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

Wouldn't it be straightforward to adjust the first line of parseBool to

if value is None:
    return None

@michaelkirk
Copy link
Contributor

Choose a reason for hiding this comment

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

Somehow I was conflating parseBool with bool e.g. bool(None). Given that we've got our own specific thing->Bool method, we might as well make it do what we want.

Pending @volaya's original motivations for this change, I'd say @m-kuhn's suggestion to alter parseBool is good 👍

@volaya
Copy link
Contributor Author

@volaya volaya commented on 946f4e4 Jan 8, 2016

Choose a reason for hiding this comment

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

@michaelkirk I changed it because of this

8798c42

Basically, as it was before, it was not correctly working for algorithms that are described in a text file (SAGA, GRASS), since the value passed was a text string with "True" or "False" and that was always understood as a true due to the non empty string. Passing that string through the parseBool method fixed that.

@michaelkirk
Copy link
Contributor

Choose a reason for hiding this comment

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

@volaya Can you point me to the text file parsing code?

@volaya
Copy link
Contributor Author

@volaya volaya commented on 946f4e4 Jan 12, 2016

Choose a reason for hiding this comment

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

@michaelkirk https://github.com/qgis/QGIS/blob/master/python/plugins/processing/core/parameters.py#L47

That method is the one that Jurgen fixed to solve the issue with the test

Please sign in to comment.