builtin checker findings for mslib/plugins/io/csv.py and mslib/utils/time.py

[ReimarBauer]

@matrss seems the builtin checker was updated?

I rather don't want to rename the csv io plugin, the time utils module can maybe refactored renamed to times (just plural?)

@joernu76 What do you think?

I assume we need to solve this in stable.

Successfully installed flake8-7.0.0 flake8-builtins-2.4.0 mccabe-0.7.0 pycodestyle-2.11.1 pyflakes-3.2.0
mslib/plugins/io/csv.py:0:1: A005 the module is shadowing a Python builtin module "csv"
mslib/utils/time.py:0:1: A005 the module is shadowing a Python builtin module "time"
2     A005 the module is shadowing a Python builtin module "csv"
2

[matrss]

@matrss seems the builtin checker was updated?

Yes, there was an update on 2024-03-29 which added this lint rule.

I rather don't want to rename the csv io plugin

What's the reason for that? Is it a public API?

[ReimarBauer]

the csv plugin is used in many users configurations. It is for storing / reading in a different format a flight track.

When we rename this we need to check if we need a migration script.

https://mss.readthedocs.io/en/stable/plugins.html

on default we have import enabled, but export can be enabled by user configuration. Looks like a migration script then or we ignore this one.

[ReimarBauer]

in stable we should ignore both, because a fix is an API change.

[matrss]

OK, so it is part of the public API (I don't think we define what is and isn't, right? Maybe that should be made clear at some point). Honestly, I would be fine with just ignoring those two specific instances of this issue (until they actually become a problem), but leave the lint rule enabled so we don't inadvertently introduce more.

[matrss]

When we rename this we need to check if we need a migration script.

Another thought: a migration script would be the wrong thing to do IMO, since you would have to parse python to do that (and due to pythons dynamic nature you probably still couldn't tell if a program uses this module without executing it with every possible configuration).

A deprecation warning on import and a complete removal in a later release would be more sensible.

[ReimarBauer]

We have to migrate the users entries in the msui_settings.json related to our code changes. json not python. We did such migration already in the past.

But I think we should keep what we currently have on module definitions and wait for a real problem or until the plugin concepts gets refactored. In principle the docking widgets could also be plugins. At the moment there are more contributions for such parts we likly want to have these also pluggable. At this time we would also change the naming scheme for those IO plugins.

It is a good idea to leave the checker enabled to not introduce more conflicts.

[ReimarBauer]

Yes, we should set a deprecation warning, that it will be changed in a later version. At the time we know the version we can adjust it.

It is also a good remark for us.

[ReimarBauer]

looks like we don't have this check in stable, seen that it did not fail on an other PR, so we can fix this in develop

[ReimarBauer]

oh, running that locally

mslib/plugins/io/__init__.py:0:1: A005 the module is shadowing a Python builtin module "io"
mslib/plugins/io/csv.py:0:1: A005 the module is shadowing a Python builtin module "csv"
mslib/utils/time.py:0:1: A005 the module is shadowing a Python builtin module "time"

[ReimarBauer]

hmm, it is not good that we named that io, but since when is the io library a builtin?

[ReimarBauer]

We don't touch io plugins for this release but refactor time. #2315

I don't think we need a deprecation warning, but we can do in stable. The new name is time_handling.
I will later add a HINT in CHANGES. There is one occurancy in wms.py.