gh-358: Add mypy CI and config

[erlend-aasland]

Estalish a minimal mypy config that ensures annotations are compatible
with Python 3.8 (the oldest supported Python version).

Add a forgiving CI job that does not affect the status of the GitHub
workflow run.

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.22%. Comparing base (6bc90a8) to head (5b8d3e9).

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #518      +/-   ##
==========================================
+ Coverage   71.16%   71.22%   +0.06%     
==========================================
  Files          26       26              
  Lines        3114     3114              
  Branches      527      480      -47     
==========================================
+ Hits         2216     2218       +2     
+ Misses        766      765       -1     
+ Partials      132      131       -1     

see 2 files with indirect coverage changes

[erlend-aasland]

This shows up red in the PR list:

... and green in the Actions log:

The former might be mitigated (and thus made green) by flipping some project configuration settings and making it a non-required check; I'm not quite sure where that setting lives, though.

[acolomb]

@christiansandberg seems like only you have the necessary access privileges to configure repository settings. I can look into the matter if you give me the permissions.

[christiansandberg]

Do you have some instructions on how to configure that? It also seems I can't change the level of access contributors have in GitHub either.

[acolomb]

The settings for checks should be somewhere under branch protection for the master branch. Sorry, I don't have any repository at hand where I could search the setting myself. None where I am the owner and checks are defined...

The level of access needed would be to change repository settings, but I can't tell how that would be assigned.

[christiansandberg]

Can't see that it is possible to sort of "white-list" a specific step so that the whole action becomes green.

It might also be hard for a new contributor to know what to do about the failed step since they can't fix it.

[erlend-aasland]

Can't see that it is possible to sort of "white-list" a specific step so that the whole action becomes green.

I suspect the mypy CI would have to live in its own YAML file in order to be selectable.

[acolomb]

I've asked the folks at Syncthing, where I've seen such a setup: https://forum.syncthing.net/t/ot-question-about-github-checks/22577

[acolomb]

@christiansandberg does this help? https://forum.syncthing.net/t/ot-question-about-github-checks/22577/2?u=acolomb

It might not show the new check yet, as it is not merged to master. We could set up a separate branch to test it though.

[christiansandberg]

Yes, it should show up once this is merged to master I hope.

[christiansandberg]

But I guess it won't be any different compared to now since status checks do not block merging? I think it still may cause confusion if you don't know that some checks are OK to fail.

[acolomb]

IIRC, the pull request will be marked with a green checkmark although such an optional check is failing. In the list of checks, all but the mypy check will get a "Required" label attached.

This all depends on actually enabling the branch protection of course, to disallow merging with failing checks at all. Which is a good idea in general. The second step is then to exempt the specific mypy check as "non-required".

Once we actually have fixed it to not fail, we can remove that exemption again - or leave it active as I think typing checks are still somewhat icing on the cake and could be handled separately after merging a feature PR.

If all this cannot be accomplished with the GitHub settings, then we might just as well concentrate on fixing the actual errors before enabling the CI check. But let's try getting it to work first 😉

[erlend-aasland]

Yes, it should show up once this is merged to master I hope.

Yes, I think you're right that it needs to be on a protected branch, such as master, in order for it to be selectable. From your screen-shot, it seems to me it does not have to be defined in a separate YAML file, since you already get to choose from the feature matrix in the CI.

Anyway, let's hold this PR until CI is green again (see commit d1c28e5).

[erlend-aasland]

[...] We could set up a separate branch to test it though.

That's a very good idea.

[acolomb]

I've created https://github.com/christiansandberg/canopen/tree/mypy with this PR merged into it. It doesn't run the actions at all though, probably CI is only enabled for master. Could use that as a playground to fiddle with the settings though. If you're so inclined @christiansandberg 😉

[erlend-aasland]

I've created https://github.com/christiansandberg/canopen/tree/mypy with this PR merged into it. It doesn't run the actions at all though, probably CI is only enabled for master. Could use that as a playground to fiddle with the settings though.

Nice, thanks! I've created a test PR against it:

• TEST: mypy CI test #528

[acolomb]

@christiansandberg it still would help if you could give me access to the repository settings, so I can adjust the protection rules directly. At least until we have found the right configuration.

[erlend-aasland]

@christiansandberg it still would help if you could give me access to the repository settings, so I can adjust the protection rules directly. At least until we have found the right configuration.

Gentle ping, @christiansandberg 🏓

[acolomb]

Alrighty, looks like this configuration works now for the mypy branch. The typing check is run and displays as failed, but does not block the PR from being merged. In contrast, failing the usual build or docs jobs does block from merging a PR. See #528 for an example.

Now what's the status on this one and #528, do we need to merge more than this PR alone?

[acolomb]

It looks a little strange to see "Python package / mypy (pull_request)" as the title for this check. I'm leaning towards moving it to a separate workflow file. Or is there any particular reason why it should be linked with the pythonpackage.yml?

Either way, we should give it a more appealing name such as - name: Run mypy static type checker

For the possibly new workflow file, we could use a more generic name / title and keep in mind that more checkers / linters might be added there later. Something like "Check correctness" or "Static code analysis".

[acolomb]

Well the latest iteration leads to the mypy checking step never failing. Therefore a commit gets a green checkmark if all other jobs succeed. Not sure if this is the better trade-off. Opinions?