Ruff linting

[maarten-ic]

Enable additional ruff linting rules

Most changes are trivial, but please check if you agree with my solution for the base ymmsl.document.Document class:

• Rules triggered by ruff:
  1. B024 Document is an abstract base class, but it has no abstract methods or properties
  2. B027 Document.__init__ is an empty method in an abstract base class, but has no abstract decorator
• I tried making __init__ an abstract method (and add a concrete __init__ method to ymmsl.v0_1.document.Document) but then mypy complains: ymmsl/io.py:34: error: Only concrete class can be given where "type[Document]" is expected [type-abstract]
  So instead I completely removed the ABC superclass from document.Document.

Suggested to review commit-by-commit and don't spend too much time at the import sorting commit 🙂

[LourensVeen]

Document needs to be abstract somehow, so we'll have to figure out how to do that, or if all else fails apply some overrides. I'll play with it a bit.

Update: looked at it and we're running into a long-standing mypy issue that I've had pop up before. Basically Type only matches concrete types, not abstract types, even though that doesn't make any sense (at least to me, and to quite a few other people in that thread). Type should really match any type, and then there should be something like ConstructibleType or ConcreteType for things that need to not be abstract. So I'm fine with type: ignore here.

[LourensVeen]

Technically this changes the format, but it's a very minor compatible addition that won't hurt anything, so okay.

[LourensVeen]

Huh? Isn't that what Python does by default? Why does ruff change this to do it explicitly then?

[maarten-ic]

When raising a new exception from within an except clause, it's recommended to include a from clause to explicitly set the exception's cause. Without it, Python will implicitly chain from the current exception (setting __context__), but the __cause__ attribute won't be set, which may make debugging slightly more difficult.

See: https://docs.astral.sh/ruff/rules/raise-without-from-inside-except/

I wouldn't mind if you want to globally disable this rule.

[LourensVeen]

Hm, well, that sounds like it actually improves things even if by a small amount, and if we're going to ruff autoformat then it doesn't cost anything then let's just leave it in.

[LourensVeen]

Document used to be non-abstract, but that ended up giving confusing error messages if the ymmsl version tag was wrong. For example, if you put ymmsl_version: v0_2 in the document, then neither v0_1.Document nor v0_2.Document would match, and yatiml would try to match ymmsl.Document, which doesn't have any keys, and you get something like this:

        ymmsl_version: v0_2
        ^
    Found a key "ymmsl_version", which is not allowed here. Maybe it was indented incorrectly? For reference, no other keys are allowed, but "ymmsl_version", "description" and "models" were given.

It's pretty easy to overlook the actual problem, especially if the error message points you in the wrong direction.

The problem there is that yatiml considers ymmsl.Document something that could be what the user intended, while it's really a base class that's only there to provide a single type that can represent any kind of ymmsl document. It really is abstract, and so it should really be declared abstract, and yatiml understands that concept and then produces a good error message:

Multiple things are allowed here, but none of them were recognised correctly. At least one of these errors should apply to what you want to do; please solve that one and ignore the others.
  in "<unicode string>", line 1, column 1:
    ymmsl_version: v0_2
    ^
Incorrect attribute value v0_2 where v0.1 was required

  in "<unicode string>", line 1, column 1:
    ymmsl_version: v0_2
    ^
Incorrect attribute value v0_2 where v0.2 was required

and now it's obvious what the problem is. So this needs to be solved differently. I'll see if I can find some other way of making ruff and mypy happy.

[maarten-ic]

Thanks for clarifying, I wasn't aware of this!
Agreed with your solution to ignore the mypy error 👍

[LourensVeen]

Yeah, I've been thinking about how that should have been documented, or whether me having a vague memory of a problem with this and a private notes file from last year that I found using grep is a good enough way of preventing regressions. I could have added a test that checked for an error containing Incorrect attribute value v0_2, but I figured this wouldn't regress anyway...

[LourensVeen]

Note to self: this adds more linting rules and fixes them, but we still need another PR that ruff formats everything to match up with multiscale/muscle3#377.

[LourensVeen]

Nice fix, Lourens!