[Bug] Pytest-Pylint workflow fails

[echoix]

Describe the bug
A clear and concise description of what the bug is.
Pylint running through pytest jobs fail in CI.

Pytest released version 8 15 hours ago and some invocations are now unsupported.
We already had a plugin that was downgraded. We need to fix the underlying issues promptly.

[echoix]

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.

grass/temporal/t.vect.observe.strds/t.vect.observe.strds.py

Lines 363 to 365 in 5b2460a

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

Then in main, these options and flags variables are used

grass/temporal/t.vect.observe.strds/t.vect.observe.strds.py

Lines 83 to 93 in 5b2460a

	def main():
	# lazy imports
	import grass.temporal as tgis
	# Get the options
	input = options["input"]
	output = options["output"]
	vector_output = options["vector_output"]
	strds = options["strds"]
	where = options["where"]
	columns = options["columns"]

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

grass/python/grass/script/core.py

Line 822 in 5b2460a

def _parse_opts(lines):

Used by

grass/python/grass/script/core.py

Line 854 in 5b2460a

def parser():

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?

[echoix]

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.

[echoix]

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

[veroandreo]

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.

[echoix]

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.

[veroandreo]

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

[echoix]

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

[petrasovaa]

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()

[echoix]

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