search

GispoCoding / flake8-qgis

Flake8 plugin for QGIS python plugins
MIT License
6 stars 4 forks source link

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

Open kannes opened 8 months ago

kannes commented 8 months ago

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.

Joonalai commented 8 months ago

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 https://github.com/qgis/QGIS-Documentation/commit/56d52be05e8d24f243d4c7ddb32bbc197683736e 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.

LKajan commented 3 months ago

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