Enable mypy review for pyshacl.cli:main and fix reported issues

[ajnelson-nist]

This patch updates the results of mypy --ignore-missing-imports, run only against /pyshacl/cli.py:

Count	Type-reviewed state
0	Current master branch state
40	Only designating main() -> None
0	Committed state

This patch updates the results of mypy --ignore-missing-imports, run against /pyshacl:

Count	Type-reviewed state
20	Current master branch state
61	Only designating main() -> None in cli.py
21	Committed state of cli.py
20	Committed state including main.py

[ajnelson-nist]

I've noticed type review hasn't passed for a while. So I included in this PR evidence that the number of issues at least doesn't grow, while the type-annotated coverage does.

[ashleysommer]

This still needs to skip parse_args() if the HTTP or SERVER env variables are set.
This is to allow the user to run PySHACL in server mode, without passing any cmdline arguments.
Normally omitting cmdline arguments will cause parse_args() to throw it's missing arguments error.

[ajnelson-nist]

Restored, using an empty Namespace object instead of an empty dictionary so args has a more consistent type across both these branch sides.

[ashleysommer]

Should main() be changed to return int exit code, then pass that to sys.exit() from the caller site? That would avoid all the sys.exit() calls in here.

[ajnelson-nist]

At some point years ago, the idea wormed into my head that main() in Python had to return None.

However, I've had poor luck finding out where that idea came from. My current best guess is mypy complaining about missing return signatures when I was first learning type annotations. E.g. from this program try.py:

def main():
    x = 1
    x = "1"

if __name__ == "__main__":
    main()

mypy try.py returns no issues; fine & dandy, there's no typed context. mypy --strict catches the error I want around x, but gives this output about the rest:

try.py:1: error: Function is missing a return type annotation  [no-untyped-def]
try.py:1: note: Use "-> None" if function does not return a value
try.py:3: error: Incompatible types in assignment (expression has type "str", variable has type "int")  [assignment]
try.py:6: error: Call to untyped function "main" in typed context  [no-untyped-call]
Found 3 errors in 1 file (checked 1 source file)

I thought there used to be a specific suggestion about main instead of what's on the first 2 or last 1 lines. It might have just been me conflating nearby lines instead. I'm not finding a say on the matter in mypy's documentation, PEPs on typing (483 and 484), or other search engine results.

Since mypy --strict didn't complain at this:

from typing import Dict, Set

def main() -> Dict[str, Set[int]]:
    return {"a": {1}}

if __name__ == "__main__":
    main()

I'm fine swapping -> None with anything else if you think it'll result in cleaner chaining.

[ajnelson-nist]

I started the "main returns an int" revision, and stopped partway through revising the sys.exit(2) calls when I remembered another hint I had for why main usually returns None.

sys.exit(n) influences the return status of the program call from main. return n doesn't.

E.g. here's a file try.py:

def main() -> int:
    print("I ran.")
    return 1
main()

Here's the shell-visible exit status:

$ python3 try.py ; echo $?
I ran.
0

So I think the more shell-consistent idiom is to have the calls to sys.exit(main()) instead just be calls to main(), and to leave any deeper-in-main calls to sys.exit(n) as they are now. I don't think having main() -> int gains any control-flow advantage over sys.exit doing hard-stops with a controlled exit status value, or raise SomethingUnfortunate passing a traceback exit value 1 if uncaught.

[ajnelson-nist]

Checking back in on this thread: My own preference, in the general case for Python beyond this repository, is that main should return None, and anything meant to influence Python's exit status as non-0 should be handled with sys.exit() or raise.

I see the alternative "chaining into sys.exit" style seems to be rooted in use of pyshacl.sh_http.cli(). I'm guessing there are nuances, possibly for users outside this repository, of handling traceback and control flow the way it's being handled there. So, I could understand you preferring pyshacl.cli.main() -> int.

This seems like the last open review matter on this PR. Which is your preference - int or None for main?

[ajnelson-nist]

I checked running make dev ; make type-check in a fresh virtual environment, and it looks like the current Drone-reported failures against Python 3.8 are happening in this PR and on the master branch. And one of the type issues is something I don't quite understand with importlib, but it looks like #214 might influence that type issue's resolution.

Also, having caught up with the 0.24.0 release, this PR now appears to have no influence on the number of typing issues reported. So, the effect of the PR has become that type signatures are added.

[ajnelson-nist]

I believe after #223 is merged and this PR gets a catch-up merge, this PR will pass the type-checking Make target as it's currently written. There just seems to be the question of what the return type of pyshacl.cli.main should be.

[ajnelson-nist]

I believe after #223 is merged and this PR gets a catch-up merge, this PR will pass the type-checking Make target as it's currently written. There just seems to be the question of what the return type of pyshacl.cli.main should be.

Catch-up merge applied.

[ashleysommer]

Okay, I'm happy to merge this now

[ajnelson-nist]

Thank you!

[ashleysommer]

This is released now in PySHACL v0.26.0