Drop unsupported Python versions from CI; add new stable versions.

[richardxia]

This drops Python 3.5, 3.6, and 3.7 from the CI workflows, since they are all EOL. It adds 3.9, 3.10, and 3.11 as new versions we test against.

[richardxia]

Looks like this is still failing because we're not pinning our dependency versions in requirements.txt, and so it looks like mypy and/or pyparsing's type annotations are now stricter and causing us to fail type checking.

I'm a little bit worried that this is going to a deeper rabbit hole than either @nick-knight's or my PRs cover, so I think at this point I'll defer the untangling of CI to someone else.

[nick-knight]

It would appear that CI hasn't been run in a while! All four builds --- including 3.8, which was untouched by this PR --- are failing with the same error:

. venv/bin/activate && mypy -p pydevicetree
pydevicetree/source/grammar.py:20: error: Argument 1 to "enablePackrat" of "ParserElement" has incompatible type "int | None"; expected "int"  [arg-type]
pydevicetree/source/grammar.py:46: error: Incompatible types in assignment (expression has type "ParserElement", variable has type "Forward")  [assignment]
pydevicetree/source/grammar.py:55: error: Incompatible types in assignment (expression has type "DelimitedList", variable has type "Forward")  [assignment]
Found 3 errors in 1 file (checked 10 source files)
make: *** [Makefile:32: test-types] Error 1
Error: Process completed with exit code 2.

[richardxia]

Also, this is fun: pyparsing's type annotations are incorrect. In the latest release, 3.1.0, that enablePackrat() method (which got renamed to enable_packrat() with a compatibility shim) declares its first positional argument, cache_size_limit, as an int. However, you can see in the method body that they are explicitly handling the case where cache_size_limit is None. Furthermore, the docstring explicitly describes the behavior when a None is passed in.

[mikeurbach]

That's annoying...

It seems easy enough to do:

if cache_bound:
    p.ParserElement.enable_packrat(cache_bound)

Which seems to match the intent on our side.

I'm trying to understand the other errors and what's changed.

I guess once these are fixed, we should probably pin the dependency versions to their latest.

[nick-knight]

Not 100% sure, but might need to replace = with <<= in the the latter two errors.

When I add p.enable_all_warnings(), I get

$ python3 grammar.py
/home/knight/pydevicetree/pydevicetree/source/grammar.py:45: UserWarning: Forward defined here but no expression attached later using '<<=' or '<<'
/home/knight/pydevicetree/pydevicetree/source/grammar.py:56: UserWarning: Forward defined here but no expression attached later using '<<=' or '<<'

[richardxia]

if cache_bound:
    p.ParserElement.enable_packrat(cache_bound)

Actually, I don't think that's equivalent. When you explicitly pass None as the cache size, the method will mutate the global ParserElement's packrat_cache class variable by setting it to _UnboundedCache(). The default value for that class variable is an instance some other internal class named _CacheType.

I'm not really sure if any of this is actually important for pydevicetree's functionality or performance, but it might be safest to just disable type checking on this file entirely. Presumably when this code and the CI tests were first written four years ago, pydevicetree did not provide type annotations yet, and therefore all of this would have been treated as untyped (the Any type). This is probably why there's a # type: ignore comment on the import pyparsing as p statement at the top of this file, since normally mypy will print a warning or error letting you know that the package doesn't have any type annotations available.

[mikeurbach]

Not 100% sure, but might need to use <<= to resolve the other two errors.

Yep, I found some examples in the docs for Forward that mention <<. Looks like <<= takes care of unexpected behavior w.r.t. precedence, so that makes sense to me.

[richardxia]

I think the other two errors are due to shadowing variables using values of new types. One rule of mypy is that you cannot reuse a variable if you want to assign something of a different type to it, even if you're done using the old value. E.g., the following is a type error:

foo = 123
bar = foo + 1
foo = "hello"  # This is a type error, since you are assigning a str to a variable of type int

I bet you can fix the other two type errors by just giving the new variables new names.

[nick-knight]

I think the first error should be resolved with # type : ignore and the latter two should be resolved by replacing = with <<=. Since no PRs can pass CI without disabling the older Python versions, I think these changes will need to be made in this PR.

EDIT: sounds like <<= doesn't fix the issue.

[mikeurbach]

Ok, I'm happy to add type: ignore to the enable_packrat call, and file an issue upstream.

I'm not sure what is going on with the p.Forward() though. It seems like this is a way to express a forward declaration with pyparsing, use it in expressions, and then bind a value to the variable later. I.e., if we did:

arith_expr = p.Forward()
ternary_element = arith_expr ^ integer
ternary_expr = ternary_element + p.Literal("?") + ternary_element + p.Literal(":") + ternary_element
arith_expr_other = p.nestedExpr(content=(p.OneOrMore(operator ^ integer) ^ ternary_expr))
... use arith_expr_other

I'm not sure what happens (I know nothing about pyparsing until today).

Versus:

arith_expr = p.Forward()
ternary_element = arith_expr ^ integer
ternary_expr = ternary_element + p.Literal("?") + ternary_element + p.Literal(":") + ternary_element
arith_expr <<= p.nestedExpr(content=(p.OneOrMore(operator ^ integer) ^ ternary_expr))

Besides these typecheck errors, there are a boatload of lint and unit test failures (make test-lint and make test-unit). I'm not sure what's the best way out of this rathole. Is CI passing a pre-requisite for merge? Should we remove the mypy runtime dependency without fixing all of these issues? Perhaps we can pin the versions of dependencies to whatever the last release had, if they're at all compatible with non-EOL Python versions.

Just testing what happens if i approave

[nick-knight]

I'm approving this PR. I don't think it introduces any new bugs, and I'm not convinced that the type mismatches (now) being flagged by mypy are actually problematic. (Obviously something needs to be done to (by)pass CI.)

[mikeurbach]

I gave an approval of this PR just to see if I could merge, but it looks like master is restricted. I'm not sure who has access here; I don't have the Settings menu in the GitHub UI, so it might be by user?

[richardxia]

I don't have a Settings menu either. We'll need to either find someone who has Admin-level access to this repo, or Owner-level access to the organization.

Even if we fixed all the type and lint issues, we still can't merge without finding an admin because the Python 3.5, 3.6, and 3.7 checks are still labeled as "required", so that admin would at least need to change the set of required checks.

[mikeurbach]

Here are the changes for mypy to pass, which I think is inline with what we've circled in on: #50. This can be merged into this branch, or whatever else, as necessary. I wanted to get them off my local machine.

[mikeurbach]

I also went ahead and opened an issue and filed a PR on pyparsing about the enable_packrat type annotation, since it seems straightforward.

[mikeurbach]

Perhaps we can pin the versions of dependencies to whatever the last release had, if they're at all compatible with non-EOL Python versions.

FYI- I tried this, but it seems the old (transitive) dependency on typed_ast doesn't compile on at least python 3.10.

[richardxia]

FYI- I tried this, but it seems the old (transitive) dependency on typed_ast doesn't compile on at least python 3.10.

Ah, right. I think as @nick-knight pointed out in an internal Slack thread, typed_ast is deprecated/unsupported, as it has been replaced with the standard library ast module. This is probably all moot if we disable CI/type checking/lint for now, but for future reference, if we needed to conditionally pin a dependency only for specific versions of Python, it is possible to do that in requirements.txt. See https://pip.pypa.io/en/stable/reference/requirements-file-format/#example for an example.

[richardxia]

As discussed offline, since we know have admin access to this repo, we're going to merge this and #48 by bypassing the CI checks, and we'll deal with getting CI healthy again later.