Skip to content

Commit

Permalink
[processing] better handling of variables in scripts
Browse files Browse the repository at this point in the history
  • Loading branch information
@volaya
volaya committed Apr 26, 2016
1 parent 5cc2dcd commit 8a9cb05
Showing 1 changed file with 9 additions and 5 deletions.
14 changes: 9 additions & 5 deletions python/plugins/processing/script/ScriptAlgorithm.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@
from processing.core.outputs import OutputFile
from processing.core.outputs import OutputDirectory
from processing.core.outputs import getOutputFromString
from processing.core.ProcessingLog import ProcessingLog
from processing.script.WrongScriptException import WrongScriptException
from processing.core.GeoAlgorithmExecutionException import GeoAlgorithmExecutionException

Expand Down Expand Up @@ -321,16 +322,19 @@ def processAlgorithm(self, progress):
ns[out.name] = out.value

variables = re.findall("@[a-zA-Z0-9_]*", self.script)
print variables
script = 'import processing\n'
script += self.script

scope = QgsExpressionContextUtils.projectScope()
projectScope = QgsExpressionContextUtils.projectScope()
globalScope = QgsExpressionContextUtils.globalScope()
for var in variables:
varname = var[1:]
if not scope.hasVariable(varname):
raise GeoAlgorithmExecutionException("Wrong variable: %s" % varname)
script = script.replace(var, scope.variable(varname))
if projectScope.hasVariable(varname):
script = script.replace(var, projectScope.variable(varname))
elif globalScope.hasVariable(varname):
script = script.replace(var, globalScope.variable(varname))
else:
ProcessingLog.addToLog(ProcessingLog.LOG_WARNING, "Cannot find variable: %s" % varname)

exec((script), ns)
for out in self.outputs:
Expand Down

8 comments on commit 8a9cb05

@m-kuhn
Copy link
Member

@m-kuhn m-kuhn commented on 8a9cb05 Apr 26, 2016

Choose a reason for hiding this comment

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

Hi Victor,

What will happen if I expect from a python script that a variable has been defined in the project but it has not been done by the user? How will error handling happen?

I.e. is this approach precise enough to be triggered always when it's required and to not cause any unwanted side-effects.

Some ideas for alternative approaches:

Would it possibly be better to define an expression context with these variables on top of the script (just the same way import processing is injected) and a scripter could then use

QgsExpression('@some_crazy_variable').evaluate(expressionContext) # expressionContext injected into the script

Or even better a decorator for a script function that does this magic (it will less look like black magic)

@injectExpressionContext
def myScriptFunction():
    expressionContext.variable('default_resolution')

Ping @NathanW2 for all the python goodies and @nyalldawson for the expression details

@m-kuhn
Copy link
Member

@m-kuhn m-kuhn commented on 8a9cb05 Apr 26, 2016

Choose a reason for hiding this comment

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

@volaya seems this commit has broken the tests:

https://travis-ci.org/qgis/QGIS/jobs/125798185#L1486

Can you check the indentation?

@volaya
Copy link
Contributor Author

@volaya volaya commented on 8a9cb05 Apr 26, 2016

Choose a reason for hiding this comment

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

If the variable doesn't exist, it will warn the user and the code will be executed without replacing the variable. If it fails, the error will be reported accordingly by Processing, just like it happens when there is any syntax error.

Sounds like a rather logic behaviour to me.

Other solutions might be different, but i guess that the main idea of this feature is to provide a very simple way of using variables. If the scripter needs to use some more complex code, that makes not much different to what was possible before, since you can from the script get the values of a variable using the context and scope classes.

Feel free to change the code with a different approach if you think it is more solid. This is a rather advanced functionality, and it is not documented yet, so it's quite safe to play with the implementation until we find an optimal solution.

Thanks!

@m-kuhn
Copy link
Member

@m-kuhn m-kuhn commented on 8a9cb05 Apr 27, 2016

Choose a reason for hiding this comment

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

Couldn't there be collisions between (@-prefixed) decorators and variable substitution?

@volaya
Copy link
Contributor Author

@volaya volaya commented on 8a9cb05 Apr 27, 2016

Choose a reason for hiding this comment

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

Good catch. WE could use a different prefix, but it would be a different syntax to the regular one used in other parts of QGIS.

I guess we should think about how many people might use a decorator in a Processing script, and the odds or using a decorator with exactly the same name as one of the variables.

@m-kuhn
Copy link
Member

@m-kuhn m-kuhn commented on 8a9cb05 Apr 27, 2016

Choose a reason for hiding this comment

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

or make a shortcut function var('my_variable') - or even more mighty exp('3 * @my_variable')?

@NathanW2
Copy link
Member

@NathanW2 NathanW2 commented on 8a9cb05 Apr 27, 2016 via email

Choose a reason for hiding this comment

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

@nyalldawson
Copy link
Collaborator

Choose a reason for hiding this comment

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

@NathanW2 agreed. This current approach also breaks the ability to run the script as pure python without preprocessing it.

I'd suggest adding a method in the processing utils evaluateVariable( var_name, context = None) . this method would check if an existing expression context is passed and if not, construct a new one using the global, project (and possible processing specific?) scope.

Please sign in to comment.