[Bug] Pytest-Pylint workflow fails about grass HOT 9 OPEN

Dear @echoix, when reporting a bug, please remove the unused parts of the bug report template, otherwise the important stuff gets lost in the template noise.

from grass.

no worries, all good! You're doing an amazing job fixing a lot of things here and there! 🚀 It's just my ocd 🤦‍♀️

from grass.

We need a new or better pattern for grass.script.core.parser usage in modules.

Currently, something like this is suggested

        if __name__ == "__main__":
            options, flags = grass.parser()
            main()

Lets take one of the newly-failing modules, that would need to pass a Pylint version >2.12 to upgrade the pytest-pylint plugin version, t.vect.observe.strds

https://github.com/OSGeo/grass/blob/5b2460a227e3f5bd358744d8e62e5ae3707a3ad9/temporal/t.vect.observe.strds/t.vect.observe.strds.py

When the module is called, grass.script.core.parser is called before main, and sets options and flags in the global scope, then calls main.

Then in main, these options and flags variables are used

The problem is that options and flags aren't really defined before use, except if main is called ONLY when the script is ran as a file. The file can't be imported and call the function.

Some possibilities to fix this:

• Declare global variables in all the files to hold this (I don't suggest this)
• Add options and flags as arguments to the main functions of the modules, and call main(options, flags)
• Add options and flags keys used as arguments, along with *args, **kwargs and call main(**options, **flags) on all the modules.

In the middle, there's the possibility of making some things optional or doing a check if something global exists, but having a function explicitly naming its requirements is a good thing, and it allows to test it way better.

I also suggest starting using types annotations, and the
_parse_opts

Is a good candidate, as we know for sure that it returns a tuple of two dicts (mappings), one is a mapping of string to string (for options), and the second one (flags) is a mapping of string to bool. After that tools and IDEs can start helping out, since then calling parser has the types known, and then the return variables have type info, that can be used for the modules, etc...

What are the thoughts of Python knowledgable members? @wenzeslaus maybe?

from grass.

Other warnings I see are ones that would have been fixed with a newer black 23 version, but Friday black 24.1.0 with a new style was published, and last night/ this morning black 24.1.1 was released with a fix for long path names of their cache paths for systems (like Windows) which doesn't have long path names enabled by default.

Something like where previously the string was split into two lines, then later on it fit on a single line, but the two double quotes joining them together are still there.

So I'm fixing both at the same time.

from grass.

I also saw that the g.parser docs have a Python example, saying it was Python 3, but it is clearly Python 2 syntax, since it uses print as a language keyword instead of a function

from grass.

I understand, I cleaned now. I know how to fill out complete bug reports, but that was only a really quick note to flag a just broken pipeline, and acknowledge it. I was afterwards working urgently on patching it, and I didn't finish the real fix as of yesterday night.

from grass.

Part of the solution for this and #3205 that I was working on is to bump Pylint, pytest, and pytest-pylint versions, and my nearly month-old quick PR #3331 is kind of becoming a requirement, since there are newer rules that doesn't make sense to ignore them when the fixes are available. I'm sure there will be more found later but I just hope it won't take another month when they are ready to solve

from grass.

We have been trying to use this, but it hasn't become part of documentation yet:

def main():
    options, flags = gs.parser()
    ...

if __name__ == "__main__":
    main()

from grass.

Still open, since version pinning isn't solving the problem itself

from grass.