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

[Processing] Optional parameters for script and model #2396

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@rldhont
Contributor

rldhont commented Oct 23, 2015

In processing core every parameters can be optional but in scripts there is no way to define a parameter as optional and in modeler only layers can be optional.

For script, I propose a notation like output, if a parameter token starts with optional the parameter is optional.

For model, I propose to add the required combobox to all parameters.

This proposition does not change the value setter.

This proposition can fix the issue http://hub.qgis.org/issues/5488.

@volaya

This comment has been minimized.

Contributor

volaya commented Oct 24, 2015

The PR looks fine, it wont break anything and it will allow script to have optional layers. That is good.

However, the rest of the changes are rather useless. With this change, you can define an optional parameter of type number or string (by makin param.optional = True)...but that will have no effect. The optional variable is only checked for layers, and Processing wont let you add that script in a model if there is not a value for that parameter, because it now ignores the optional property

So, in shor, this loks good, but the core of Processing will require more changes to make this effective.

I would suggest merging the changes for the scripts, but don't add the "required" combo box for models parameters until all those other changes have been implemented

I will be happy to help on that if needed

Let me know you opinion

@rldhont

This comment has been minimized.

Contributor

rldhont commented Oct 26, 2015

H @volaya, thanks for your review.

I'd like to update the core of Processing to have optional in models. Which changes do you think I have to do ?

I have noticed that if parameter value is set to None and the parameter is optional, the value will be:

  • Vector : None
  • Raster : None
  • Table : None
  • MultipleInput : None
  • Selection : default index, the default value is by default set to 0
  • Boolean : default value, the default value is by default set to True
  • Extent : default value, the default value is by default set to '0,1,0,1'
  • File : '' (an empty string for the path)
  • Number: default value, the default value is by default set to 0.0
  • TableField: True, (the value becomes the optional status)
  • String: '' (an empty string and not the default value)
  • CRS: default value, the default value is by default set to 'EPSG:4326'

I have time to make some changes, just let me know what I have to do.

@volaya

This comment has been minimized.

Contributor

volaya commented Oct 28, 2015

ACtually, I am seeing that most parameters have an optional parameter, so should be easy to do this

Basically, you have to make sure that the setValue method of all parameters accepts a None value and does not complain in case they are optional (but returns False if the parameter is not optional)

see for instance here

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

Let me know if you have questions (here or by email)

@rldhont

This comment has been minimized.

Contributor

rldhont commented Oct 29, 2015

Hi @volaya,

Do you think that the value for optional parameter has to be:

  • Selection : None ?
  • Boolean : None or default value ?
  • Extent : None ?
  • Number: None ?
  • TableField: None ?
  • CRS: None or default value ?

Thanks

@rldhont

This comment has been minimized.

Contributor

rldhont commented Oct 29, 2015

Hi @volaya

I updated the code to manage optional parameters.

What do you think about it ?

@rldhont rldhont added the For Review label Oct 29, 2015

@rldhont rldhont added this to the 2.14 milestone Oct 29, 2015

rldhont added some commits Oct 23, 2015

[Processing] Optional parameters for script and model
In processing core every parameters can be optional but in scripts there is no way to define a parameter as optional and in modeler only layers can be optional.

For script, I propose a notation like output, if a parameter token starts with optional the parameter is optional.

For model, I propose to add the required combobox to all parameters.

This proposition does not change the value setter.

This proposition can fix the issue http://hub.qgis.org/issues/5488.
[Processing] Manage optional parameters
Can't set parameters to null (None or '') if the parameter is not optional
@volaya

This comment has been minimized.

Contributor

volaya commented Nov 4, 2015

I took your branch and added some changes. It is already in master. Could you check that it works fine and then close this PR?

let me know if you have questions

@rldhont

This comment has been minimized.

Contributor

rldhont commented Nov 4, 2015

Thanks @volaya. I'll close this PR.

@rldhont rldhont closed this Nov 4, 2015

@rldhont rldhont deleted the rldhont:processing_optional_parameters_script_model branch Nov 26, 2015

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