Enable more ruff lint rules

[martinhoyer]

There are too many code changes required to make the checks pass and I don't really expect all of them to be accepted. It should be easy to drop whatever is deemed unnecessary, too strict or just not suitable for this project - probably better to review on per-commit basis.
I've tried to run the default set of tests, but my machine ran out of disk space 🤡, not going to claim I didn't break anything.

• Polarion report UTC timezone
  Not sure if make sense, but from what I understand the time stamp is being used to generate Title of the test run. Without timezone, I assume the datetime is naive, dependent on the OS settings/location of where the tmt is being run, resulting in potential mismatches, no?

• Would it be beneficial to add a couple of lines in the contribute doc about ruff?
  Maybe only mention when it's acceptable to add noqa (plus to add a comment why it's being added) and/or add a link to ruff rules page to keep it succinct?
  Alternatively, make it more verbose and also mention mypy?

• Isort change being explained here

• Can $maintainer(s) check the remaining, commented-out ruff rules in pyproject.toml. I think it would be nice if we could delete those not suitable for this project and keep only those that might get implemented in the future...Or just remove all of them.

Notes for some of the disabled checks:

TRY200, B904: Within an except clause, raise exceptions with raise ... from err or `raise from None.
I've actually tried to implement PEP-3134, as it looks like a good practice to me, but ultimately figured it's best to ask about it first.

Example of change in existing code:

    except json.decoder.JSONDecodeError as error:
        raise GeneralError(f"Invalid json syntax: {error}")

json.decoder.JSONDecodeError: Expecting property name enclosed in double quotes: line 1 column 2 (char 1)

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "<stdin>", line 5, in <module>
tmt.utils.GeneralError: Invalid json syntax: Expecting property name enclosed in double quotes: line 1 column 2 (char 1)

changed to:

    except json.decoder.JSONDecodeError as error:
        raise GeneralError("Invalid json syntax") from error

json.decoder.JSONDecodeError: Expecting property name enclosed in double quotes: line 1 column 2 (char 1)

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "<stdin>", line 5, in <module>
tmt.utils.GeneralError: Invalid json syntax

Q: flake8-quotes (Q000 Single quotes found but double quotes preferred) would need changes in basically whole codebase:

ruff . --select Q000
Found 4776 errors.

C90: mccabe (complex structure)

[tool.ruff.mccabe]
max-complexity = 14

$ ruff . --select C90
tmt/beakerlib.py:76:9: C901 __init__ is too complex (17 > 14)
tmt/beakerlib.py:185:9: C901 fetch is too complex (19 > 14)
tmt/convert.py:191:5: C901 read_datafile is too complex (33 > 14)
tmt/convert.py:380:5: C901 read is too complex (41 > 14)
tmt/convert.py:616:5: C901 read_nitrate is too complex (31 > 14)
tmt/convert.py:758:5: C901 read_polarion is too complex (16 > 14)
tmt/convert.py:863:5: C901 read_nitrate_case is too complex (26 > 14)
tmt/convert.py:1046:5: C901 relevancy_to_adjust is too complex (16 > 14)
tmt/export/init.py:295:5: C901 check_md_file_respects_spec is too complex (20 > 14)
tmt/export/nitrate.py:95:5: C901 convert_manual_to_nitrate is too complex (16 > 14)
tmt/export/nitrate.py:358:5: C901 export_to_nitrate is too complex (60 > 14)
tmt/export/polarion.py:150:5: C901 export_to_polarion is too complex (41 > 14)
tmt/options.py:237:5: C901 create_method_class is too complex (17 > 14)
tmt/steps/discover/fmf.py:250:9: C901 go is too complex (57 > 14)
tmt/steps/provision/mrack.py:69:5: C901 import_and_load_mrack_deps is too complex (24 > 14)

ANN: flake8-annotations - Some of the checks could compliment mypy.

S: flake8-bandit - Security/vulnerability checks tmt probably doesn't need?

ISC: ISC001 and ISC003 is actually passing already. Not sure if/how to adapt the code for ISC002 (Implicitly concatenated string literals over multiple lines)

[happz]

After a brief reading, there are some checks I find arguable, I have questions WRT to some changes, but in general, the majority looks beneficial though, +1 to this effort from me.

And here comes the "but"... I'm afraid it might be way too big for a reasonable review, as a single PR :( A collection of PRs, one for a commit/a group of similar commits (like 9ffff4d + ed34b66) - commits do seem to be very nicely rounded - would be easier to digest. Well, at least for me, YMMW. And we could discuss whether we do or do not enable a particular check.

[happz]

After a brief reading, there are some checks I find arguable, I have questions WRT to some changes, but in general, the majority looks beneficial though, +1 to this effort from me.

And here comes the "but"... I'm afraid it might be way too big for a reasonable review, as a single PR :( A collection of PRs, one for a commit/a group of similar commits (like 9ffff4d + ed34b66) - commits do seem to be very nicely rounded - would be easier to digest. Well, at least for me, YMMW. And we could discuss whether we do or do not enable a particular check.

TBH, I'm even scared to start adding comments as I worry it might be very hard to address them e.g. in the middle one of those commits :)

[happz]

Typo in a commit message: Fix flake8 C408: unnecessary collecion calls. => s/collecion/collection

[happz]

• Polarion report UTC timezone
  Not sure if make sense, but from what I understand the time stamp is being used to generate Title of the test run. Without timezone, I assume the datetime is naive, dependent on the OS settings/location of where the tmt is being run, resulting in potential mismatches, no?

Makes sense to me, but I'm no Polarion user. Sticking to UTC seems like a way less prone to misunderstandings.

• Would it be beneficial to add a couple of lines in the contribute doc about ruff?
  Maybe only mention when it's acceptable to add noqa (plus to add a comment why it's being added) and/or add a link to ruff rules page to keep it succinct?
  Alternatively, make it more verbose and also mention mypy?

Absolutely, with new linters & influx of waivers, let's communicate "the rules". Especially the comment with check ID & justification needs to be mentioned. I'd maybe add a new section before https://github.com/teemtee/tmt/blob/main/docs/contribute.rst#tests as pre-commit, linters involved, links and rules would consume a bit more than a single line, and they don't fit the tmt-ish nature of the "Tests" chapter. On the other hand, "Tests" does mention unit tests, maybe we could just structure "Tests" a bit - linters, unit tests, testsuite.

• Can $maintainer(s) check the remaining, commented-out ruff rules in pyproject.toml. I think it would be nice if we could delete those not suitable for this project and keep only those that might get implemented in the future...Or just remove all of them.

+1 for the former option.

Notes for some of the disabled checks:

TRY200, B904: Within an except clause, raise exceptions with raise ... from err or `raise from None. I've actually tried to implement PEP-3134, as it looks like a good practice to me, but ultimately figured it's best to ask about it first.

I believe raise ... from ... is the current policy. tmt has a custom exception printer, exceptions logged will look different than what would the native interpreter logged, but it does support (and expect) exception chaining. While I did add from ... to all places I noticed, I did not touch messages of these exceptions - I agree raise GeneralError(f"Invalid json syntax: {error}") => raise GeneralError(f"Invalid json syntax}") from error is much better, it may result in a patch a bit more verbose, changing a lot of places. +1 from me, I'd just recommend making it a dedicated patch, or even a series of patches, e.g. one for each subpackage or something, if it turns out to be a very common issue. Keep @psss in the loop, BTW, he's often commenting on consistent logging and error messages, such a change fits the description pretty well :)

Q: flake8-quotes (Q000 Single quotes found but double quotes preferred) would need changes in basically whole codebase:

ruff . --select Q000
Found 4776 errors.

Yeah, probably not a good candidate for implementation... (Especially when I for one prefer single quotes :)

C90: mccabe (complex structure)
Something like pylint, right? I'd leave this one for very long winter evenings, although it may serve as a pointer to way too complicated code, it might be cumbersome to get the threshold good enough.

ANN: flake8-annotations - Some of the checks could compliment mypy.

I agree. I'm experimenting with pyright, to get another pair of eyes, but it does not play well with pre-commit...

S: flake8-bandit - Security/vulnerability checks tmt probably doesn't need?

Depends. I'd give it a try, let's see what it reports, we can always disable some or even all checks.

ISC: ISC001 and ISC003 is actually passing already. Not sure if/how to adapt the code for ISC002 (Implicitly concatenated string literals over multiple lines)

Hm, here I'm worried about readability. Might be better for our code to use multiline strings & dedent() instead of concatenating literals, but seems very much like a case-by-case area.

My thoughts on the rest of the commented-out checks:

• "ARG", # flake8-unused-arguments - sounds good, but my experience tells me this particular check is mostly a waiver factory. I'd still give it a try before dropping it completely.
• "BLE", # flake8-blind-except - I think we want this one
• "FBT", # flake8-boolean-trap - no idea, let's try it out
• "A", # flake8-builtins - let's try it out
• "EM", # flake8-errmsg - seems to me dubious at best...
• "G", # flake8-logging-format - tmt has its own logging methods, this might cause false positives, but let's give it a try - I see no use for G001 or G004 but G201 or G202 might be just an extra cheap bit of sanity checking
• "T20", # flake8-print - I'd enable these too, but there will be code to fix for sure.
• "SLF", # flake8-self - in the ideal world, yes, there will be violations as of now. I'd keep it around, and fix the reported issues.
• "TCH", # flake8-type-checking - sounds like a good idea to try out, avoiding unnecessary imports, and keeping type-only imports in check...
• "PLW", # pylint-warning - let's try it out, the list of checks looks solid
• "TRY", # tryceratops - let's try it out, I have no idea what it might report, never heard of this linter, but checks look interesting

[martinhoyer]

Resolved new conflicts with main, @happz's remarks and updated ruff version, as it fixed one noqa.

[psss]

I've read through all the changes, looks good. Added just a few minor comments/questions.