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

Document reasoning for QGS105: Don't pass QgisInterface as an argument #8

Open
kannes opened this issue Nov 29, 2023 · 2 comments · May be fixed by #14
Open

Document reasoning for QGS105: Don't pass QgisInterface as an argument #8

kannes opened this issue Nov 29, 2023 · 2 comments · May be fixed by #14
Labels
enhancement New feature or request

Comments

@kannes
Copy link

kannes commented Nov 29, 2023

https://github.com/GispoCoding/flake8-qgis#QGS105 says "Don't pass QgisInterface as an argument"

qgis.utils is not really documented anywhere and I wonder where the suggestion to prefer importing iface from it comes from. I've seen it elsewhere but I don't remember if any reasoning was given.

In plugins using the import is obviously easier and less code than passing the QgisInterface object from the plugin's class factory around everywhere where you need it. But is this approach better? After all QGIS does automatically pass the object to the plugin. Is it the preferred way of the core developers and will it be the preferred way in QGIS 4 maybe? Or is it just a variable exposed by qgis.utils by accident?

It would be great if the rule set would not only say what but also why.

@kannes kannes added the enhancement New feature or request label Nov 29, 2023
@Joonalai
Copy link
Contributor

Thank you for the issue. I think that the idea for using qgis.utils in the first place came from PyQGIS Developer Cookbook. E.g. in remove toolbar example iface is imported from there:

from qgis.utils import iface

toolbar = iface.helpToolBar()
parent = toolbar.parentWidget()
parent.removeToolBar(toolbar)

# and add again
parent.addToolBar(toolbar)

I noticed that in the newer version of the cookbook the import line is missing. The change was done in commit qgis/QGIS-Documentation@56d52be but no explanation was given. I think that the reason is that you don't have to import (or pass) iface around when writing scripts in QGIS itself.

As you said, it is much easier to import it and it's easier to mock it as well when writing tests. It is not wrong to pass it around but it is easier to choose just one approach in plugin. What do you think, should there be another rule for warning about importing iface and recommending to pass it as argument instead? Developer could then choose which approach to use.

Anyway, I agree that this should be documented better in README.mde and maybe the error message for QGS105 should be clearer as well.

Joonalai added a commit to Joonalai/flake8-qgis that referenced this issue Apr 26, 2024
Joonalai added a commit to Joonalai/flake8-qgis that referenced this issue Apr 26, 2024
Joonalai added a commit to Joonalai/flake8-qgis that referenced this issue Apr 26, 2024
Both QGS105 and QGS107 are valid ways of using iface. However, it is better to choose which one to use.
Joonalai added a commit to Joonalai/flake8-qgis that referenced this issue Apr 26, 2024
Both QGS105 and QGS107 are valid ways of using iface. However, it is better to choose which one to use.
@LKajan
Copy link
Contributor

LKajan commented May 3, 2024

I created a general issue (#13) for enhancing the documentation to be more explanatory for all rules rules.

@Joonalai Joonalai linked a pull request May 13, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
3 participants