CZ-NIC/yangson

Revert #71?

Closed this issue · 8 comments

#71 added annotations to self on lots of methods, which required from __future__ import annotations, which in turn dropped support for python < 3.7.

Is this the right trade-off?

  • python 3.6 is still in support through 2021. It remains the version that you get if you install python3 on centos 7, for example, which is where I am interested in running it

  • annotating self doesn't seem particularly valuable - that's the one argument whose type we can always be pretty sure of, and which mypy infers automatically. I don't see other projects including this annotation.

  • the type annotations in yangson don't appear to be machine-checked anyway, which is to say that they almost certainly are buggy:

mypy -p yangson
...
Found 648 errors in 17 files (checked 19 source files)
  • (if you are checking them some other way, I'd be interested to know about it!)

All in all, the value provided by the self annotations seems to me to be rather small compared to cutting off users stuck on older systems.

What do you think? Would you accept an MR that removed these annotations from #71?

cc @kwatsen

The reason why the PR is good is because the developers were previously having difficulty reading/understanding the code, which has modules containing many classes with methods of the same name. Now both browsing and stacktraces are more readable.

It's unclear if there is a need to support < 3.7. You raise a point about Centos 7, but 1) 3.7 is considered by many to be the first good Python 3 release, 2) CentOS 7 is not the current release (use Centos 8?), and 3) it is best practice to use Python virtual environments for Python projects (can you?)

I am curious as to what tooling you are using such that type annotations make a difference to stack traces.

I guess if you find annotations on self parameters more readable then you do; but the usual convention is to omit them. As I say, I don't see other projects doing this.

Alas, it's not always possible to use the latest of everything: but for what it's worth I believe that python 3.6 is also the default python3 on Centos 8.

Anyway, I await maintainer opinion.

I tend to agree with @kwatsen, it is really helpful to immediately see which version of an overloaded method one is looking at.

I have one suggestion: we could change the offending annotations so that instead of e.g.

    def foo(self: Bar):

we'd have

    def foo(self: "Bar"):

which can be used without importing from __future__.

@kwatsen would this work for you?

I prefer as is. The "Bar" form is odd, IMO. I've never ever seen it before.

And I think that Centos is behind the times. IIRC, Ubuntu comes with 3.7...

That said, the "Bar" form likely does address the root concern (assuming it shows in tracedumps) and, Lada, I understand that you may feel obliged.

Annotating self is odd and I've never seen it before IMO! But there you go.

Quoting the self annotations works for me, I can submit a fresh MR if that's acceptable to all.

Re: "odd" - jinx! ;)

I don't know what the quoted form means or where it is defined as legal syntax. I wonder if it might ever become will ever become unsupported... Unlikely, but it's so odd that I'm unsure.

FWIW, it may be that someday someone creates a static or runtime checker for the unquoted form, as the unquoted form is scheduled to become default syntax in Python 3.10)

the quoted form is all there in PEP 484 and supported by mypy and (presumably) other type-checkers.

he quoted form is all there in PEP 484 and supported by mypy and (presumably) other type-checkers.

Right, and it is also described in the docs for typing module (search for "forward reference"). Such quoted annotations had been already used in Yangson.

So, @dimbleby, please submit the MR, I think it is a reasonable compromise.