Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 5 commits into from
Mar 19, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e880e62
Enable mypy review for pyshacl.cli:main and fix reported issues
ajnelson-nist Jul 25, 2023
9c7479a
Merge branch 'master' into type_review_cli_py
ajnelson-nist Nov 13, 2023
71d16c6
Restore do_server check
ajnelson-nist Nov 13, 2023
ef4c857
Merge branch 'master' into type_review_cli_py
ajnelson-nist Mar 15, 2024
18cbe68
Merge branch 'master' into type_review_cli_py
ajnelson-nist Mar 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pyshacl/__main__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,4 +23,4 @@ def str_is_true(s_var: str):

sys.exit(http_cli())

sys.exit(main())
main()
8 changes: 5 additions & 3 deletions pyshacl/cli.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import sys

from prettytable import PrettyTable
from rdflib import Graph
from rdflib.namespace import SH

from pyshacl import __version__, validate
Expand Down Expand Up @@ -183,14 +184,14 @@ def str_is_true(s_var: str):
# parser.add_argument('-h', '--help', action="help", help='Show this help text.')


def main():
def main() -> None:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

basename = os.path.basename(sys.argv[0])
if basename == "__main__.py":
parser.prog = "python3 -m pyshacl"
do_server = os.getenv("PYSHACL_HTTP", "")
do_server = os.getenv("PYSHACL_SERVER", do_server)
if do_server:
args = {}
args = argparse.Namespace()
else:
args = parser.parse_args()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if str_is_true(do_server) or args.server:
Expand Down Expand Up @@ -306,10 +307,11 @@ def col_widther(s, w):
t2.field_names = ['No.', 'Severity', 'Focus Node', 'Result Path', 'Message', 'Component', 'Shape', 'Value']
t2.align = "l"

assert isinstance(v_graph, Graph)
for i, o in enumerate(v_graph.objects(None, SH.result)):
r = {}
for o2 in v_graph.predicate_objects(o):
r[o2[0]] = str(col_widther(o2[1].replace(f'{SH}', ''), 25)) # max col width 30 chars
r[o2[0]] = str(col_widther(str(o2[1]).replace(f'{SH}', ''), 25)) # max col width 30 chars
t2.add_row(
[
i + 1,
Expand Down