temporal: create limited connection to TGIS DB in current mapset for (un-) registering maps - #7901
temporal: create limited connection to TGIS DB in current mapset for (un-) registering maps#7901ninsbl wants to merge 7 commits into
Conversation
|
Related to #7670 |
| unit=None, | ||
| increment=None, | ||
| dbif=None, | ||
| dbif: SQLDatabaseInterfaceConnection | None = None, |
There was a problem hiding this comment.
You say you will remove this in a future version. I have two problems with that:
If you remove, you'll need to make the argument passed keyword only at least at that point of the argument list, so it isn't used as positional arguments by other code, that will break when changing versions. We looked up this pattern in July, with * and /.
Next, I'm not sure it's going to do the right thing. Usually, a design pattern named dependency injection (part of the broad IoC: inversion of control), says that you should be giving your functions all the dependencies from the outside. The finality is that the function doesn't really depend on the outside, and shouldn't be affected by global scope: way easier to test.
So, will removing it be an improvement because you'll actually don't need it, or it will be increasing the coupling with either other functions, o the global state?
There was a problem hiding this comment.
I see. The reason for me to suggest to deprecate the dbif parameter is, that it will be ignored after the change. Creating the connection is relatively cheap and if the function would take dbif as input it would have to make sure the DB interface does not contain connections to other mapsets. So disallowing dbif as input would be clearer and safer. Of all ~115 usages of the function in the code base (mainly unittests), less than a hand-full actually ever passed a real DB-interface other than None or defalt by ommission.
Unfortunately, almost all temporal functions depend on users running tgis.init() which sets a couple of globals (and creates a temporal database if it does not exist).
We could reduce the globals-dependency by using "." as input to SQLDatabaseInterfaceConnection instead of get_current_mapset() (with the latter reading from globals).
In any case, coupling with other functions or global state will at least not increase by the suggested changes - as far as I can see - because if the user passes None as dbif (before this change), dbif would be created, just like after this change, only with init_dbif function which has the same dependencies to tgis.init() as the SQLDatabaseInterfaceConnection....
That said, I removed all usage of the dbif parameter in this function in the code base, to make sure a deprecation warning would not trigger.
I am more than open to suggestion for handling the little useful dbif parameter going forward...
| "The dbif argument is deprecated and will be removed in future " | ||
| "versions. The database connection will be created automatically" | ||
| "only for the current mapset." |
There was a problem hiding this comment.
Can you double check that the first sentence is matching the pattern of already translated strings? I often get bumped to new translations of that sentence when translating, and it's bothering to always restart the same work over and over
Replaces a hack that modified a broad SQLDatabaseInterfaceConnection with a clean DB connection limited only for the current mapset.