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

Clean code and add more expression functions #8

Merged
merged 13 commits into from
Jul 2, 2017

Conversation

kharesimran
Copy link
Contributor

@kharesimran kharesimran commented May 16, 2017

Hi @NathanW2

Thanks for accepting my contributions to the qgsexpressionsplus plugin.

I have made a few changes, cleaned up the code, and added a few more custom expression functions. My supervisor, Prof Stefan Keller, suggested some more ideas for expression functions that will be nice to have in QGIS, which are included here as well.

Here is the final list:

  • jitter_geometry()
  • max_incremented()
  • get_env_variable()
  • set_env_variable()
  • hstore_exist()
  • hstore_get_value()
  • hstore_contains_key_value()
  • hstore_contains_hstore()
  • nullif()

I have added a description for the functions above in the Readme.

The hstore functions are useful when we are working with Openstreetmap data in QGIS, and the attribute table has a tags column like the one in the screenshot [1] attached. We can select features by expression using these hstore functions as hstore_contains_key_value("tags", 'amenity=>restaurant') which would select all the restaurants on the layer.

The jitter_geometry() function produces an output like [2]. I found your blog post on Generating chainage nodes in QGIS really helpful when implementing this.

If you have the time, it would be great if you could review my code, in case I could have better implemented any of these functions.

Thank you!

[1]
attribute-table

[2]
jitter

@sfkeller
Copy link

Great. Thanks Simran!

@sfkeller
Copy link

sfkeller commented Jul 2, 2017

@NathanW2: If you have any questions in order to accept this pull request, don't hesitate to ask @simran001 or me.

@NathanW2 NathanW2 merged commit 472f3e3 into NathanW2:master Jul 2, 2017
@NathanW2
Copy link
Owner

NathanW2 commented Jul 2, 2017

Looks fine. Thanks.

@sfkeller
Copy link

sfkeller commented Jul 2, 2017

Thanks! That will be useful for my former workshop about QGIS Expressions https://giswiki.hsr.ch/Workshop_QGIS_Python_GeoPython_2017
and for Andreas' forthcoming one at user conference in Nødebo
https://qgis2017.wordpress.com/presentations/#monday1 .

@sfkeller
Copy link

sfkeller commented Jul 2, 2017

Sorry, me again: I'm confused regarding the current version in this repo (0.1) and the one published at official QGIS plugin page (0.4) http://plugins.qgis.org/plugins/qgsexpressionsplus/ .

The metadata.txt in this repo https://github.com/NathanW2/qgsexpressionsplus/blob/master/metadata.txt indicates "version=0.1".

But the available version in official QGIS plugin page is 0.4 (so the forthcoming release of this new plugin version will be 0.5...)?

@m-kuhn
Copy link
Contributor

m-kuhn commented Jul 3, 2017

Nice, good job @simran001 .

One question concerning, the set/get_env_variable methods: do they work on environment variables (as the name indicates) or on qgis variables (what I think it actually does looking at the code).

What is the use case for the set_env_variable function? Do you have a concrete example, where this is required?

@sfkeller
Copy link

sfkeller commented Jul 3, 2017

@m-kuhn : You're correct, the code is about QGIS variables. I understood, that these include the variables from QProcessEnvironment, since it contains e.g. user_full_name from "global QGIS context". => Isn't this correct?

Do you suggest to rename set/get_env_variable methods to set/get_qgis_env_variable?

The method set_env_variable was introduced to enable global/shared info between more than one methods. It's also there just for completeness. Of course it should be used with care.

Looking at the code of get_env_variable(var_name) I now also realize, that it probably should be modified to get_env_variable(var_name, scope) in order to deal the situation where there exist variables with same name in more than one scope?

@m-kuhn
Copy link
Contributor

m-kuhn commented Jul 3, 2017

QGIS variables can easily be retrieved with var('variable_name') (or simply @variable_name), so I don't think there's a need for this function.

If anything, then a env('variable_name') to retrieve vars from outside QGIS would be a good idea (hint, that exists by default in QGIS 3.0)

Concerning the set- function, I am a bit afraid, that it's of limited use, because expressions are not imperative code and there is no promise about when it's being executed (only about what the result is). So even if something is within a case- or if-based branch within the expression, it might still get executed. It also will be executed for every feature, so the resulting variable will be set based on the feature that by chance was the last one to be evaluated...

@sfkeller
Copy link

sfkeller commented Jul 3, 2017

Agreed. So I'd suggest following:

  • Remove methods set/get_env_variable for now here in order to get ready for a release.
  • Later on, rewrite method get_env_variable to access vars from outside QGIS 2 (using QProcessEnvironment(?), and possibly with "forward hint" in function description that this exists by default in QGIS 3.0).

BTW: Update also https://gis.stackexchange.com/questions/236638/get-value-of-variable-in-custom-python-expression-function

@m-kuhn
Copy link
Contributor

m-kuhn commented Jul 3, 2017

Sounds good

The name in QGIS 3 is env and the implementation is trivial. I would propose to have exactly the same in expressions+ for QGIS 2, so people would not need to change anything in the upgrade for QGIS 3?

Update on gis.se sounds good, simran, could you take care of that (probably add a new answer which highlights the diff between env var and qgis var and explains how qgis variables can be accessed)?

@NathanW2
Copy link
Owner

NathanW2 commented Jul 4, 2017

@sfkeller @simran001 do you want me to make a new package for this now or wait until you have done the refactor?

@sfkeller
Copy link

sfkeller commented Jul 4, 2017

Thanks for your offering! I'd suggest you wait until we've done the refactoring.

@NathanW2
Copy link
Owner

NathanW2 commented Jul 4, 2017 via email

@kharesimran
Copy link
Contributor Author

@sfkeller @m-kuhn

Hi Prof Keller and Matthias,

Thanks for all the info!

Just to add on, regarding the get/set_env_variable functions:

  • The get_env_variable() and set_env_variable() are simple Python functions and not Expression functions. As Matthias pointed out too, expression functions are executed for each feature on the layer, so I thought they would not be too useful as expression functions.

As included in the function documentation here, a user might want to import the qgis_variables.py script and use these functions from the Python console or elsewhere.

  • Yes true, QGIS variables can be accessed using @variable_name and var('variable_name') from the expression tab itself. However, this doesn't seem to work from within a Python script or expression function. I think the use case that Prof Stefan and I wanted to implement was a custom expression function that uses Nominatim's API for reverse geocoding, passing the user's name with each request, which can be retrieved from the QGIS variable 'user_full_name'. Please find the code here.

  • In QGIS, if a variable of the same name exists in more than one scope, it is overwritten following the priority local > project > global variable. I implemented the get_env_variable(var_name) accordingly and so did not include scope as an argument.

True, get/set_qgis_variable() would be a better name for the functions. Also since they are not expression functions, they probably do not belong here.

Yes, will update the answer on gis.se, and see if I can retrieve variables from outside QGIS.

@hixi
Copy link

hixi commented Jul 5, 2017

Please allow me to chime in, I've had a look at this today. My 20¢:

  • This plugin is named "qgis expressions plus", so why include functions that are not usable as expressions/inside the expression engine?
  • rename to env:
    • Does it make sense to include a function in Python (inside the QGIS Python interpreter), that reads the environment, when getting that is as easy as os.environ.get('<variable_name>')?
    • But it would actually make sense IMHO, as stated by @NathanW2, in the Expression-Environment, where an import os is not possible without own functions, so that things like env('USER') would become easily possible?
    • @NathanW2 is it possible to import the var "function" inside python, the one that does evaluate the context. Then it would be trivial to used it instead of creating an own implementation.

@kharesimran
Copy link
Contributor Author

kharesimran commented Jul 5, 2017

@sfkeller @hixi @m-kuhn

Just found a built-in function! Please try out qgis.user.os within any expression function.

For example, qgis.user.os.getenv('HOME') returns the path to the Home directory.

Also, Prof Stefan, you had once mentioned that you would like to retrieve the path of the QGIS expressions directory. The following would do it qgis.user.expressionspath. Similarly qgis.user.userpythonhome would return the '%USERPROFILE%/.qgis2/python' path.

I tried the above on QGIS version 2.18.7.

@hixi
Copy link

hixi commented Jul 6, 2017

@simran001 does this mean from your perspective, all the code from the two functions is not neccessary anymore? If so, should it be removed?

What I'm trying to say: remove the code and the reference in the Readme to qgs_variables.py and add a new expression function named env so that we are forwards compatible to QGIS 3.

@kharesimran
Copy link
Contributor Author

@hixi yes. I have removed the two functions and created a new pull request.

@sfkeller
Copy link

@NathanW2 I'm coming back to your question above where you asked to release the plugin immediately before env() expression function becomes part of it. Now I'm not sure when we can care about this env() and another pull request. IMHO the current code and documentation seam pretty clean so far. So I propose that you release this nice extension now to the QGIS plugins web portal. I'm looking forward e.g. that it will be used in the workshop by Andreas at the forthcoming QGIS User Conf. in Nødebo.

@NathanW2
Copy link
Owner

NathanW2 commented Jul 20, 2017 via email

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.

None yet

5 participants