Improve error and warning messages (fixes #58) (#59)

* Add tests that don't use poetry to check behaviour without pytype and ipython

CHANGELOG.md

@@ -17,14 +17,15 @@ These changes are in the GitHub repository but not on [PyPI](https://pypi.org/pr
 
 ### Changed
 - run under Python > 3.10
+- improve messages
 
 ### Fixed
 - locale encoding on Windows can't read UTF-8: use UTF-8; replace characters that lead to errors
 - annotated assignment is unknown construct: ignore type annotation
 - processing of folders
 
 ### Development
-- improve tests: process a folder, use `-f`, report each construct once per line
+- improve tests: process a folder, use `-f`, report each construct once per line, don't use pytype or ipython
 - move ipython and pytype to development dependencies
 
 ## [1.2.1](https://github.com/dsa-ou/allowed/compare/v1.2b1...v1.2.1) - 2024-02-10

Makefile

@@ -13,7 +13,13 @@ lint:
 	poetry run ruff check allowed/allowed.py
 
 run_tests:
-	poetry run tests/tests.sh run
+	-@poetry run tests/tests.sh run
+	@echo; echo "sample.py (no ipython or pytype)"; echo "---"
+	-@python3.10 allowed/allowed.py tests/sample.py | diff -w - tests/sample-py.txt | diff -w - tests/sample-py-none.txt
+	@echo; echo "sample.ipynb (no ipython or pytype)"; echo "---"
+	-@python3.10 allowed/allowed.py tests/sample.ipynb | diff -w - tests/sample-nb.txt | diff -w - tests/sample-nb-none.txt
 
 create_tests:
 	poetry run tests/tests.sh create
+	-python3.10 allowed/allowed.py tests/sample.py | diff -w - tests/sample-py.txt > tests/sample-py-none.txt
+	-python3.10 allowed/allowed.py tests/sample.ipynb | diff -w - tests/sample-nb.txt > tests/sample-nb-none.txt

allowed/allowed.py

@@ -1,6 +1,6 @@
 """Check that Python and notebook files only use the allowed constructs."""
 
-__version__ = "1.3dev7"   # same as in pyproject.toml
+__version__ = "1.3dev8"   # same as in pyproject.toml
 
 import argparse
 import ast
@@ -388,6 +388,9 @@ def ignore(node: ast.AST, source: list[str]) -> bool:
 
 # ----- main functions -----
 
+read_nb = False                          # remember if a notebook was read
+read_py = False                          # remember if Python file was read
+
 
 def check_tree(
     tree: ast.AST,
@@ -494,14 +497,18 @@ def check_file(
     filename: str, constructs: tuple, check_method_calls: bool, report_first: bool  # noqa: FBT001
 ) -> None:
     """Check that the file only uses the allowed constructs."""
+    global read_nb, read_py
+
     try:
         with Path(filename).open(encoding="utf-8", errors="surrogateescape") as file:
             if filename.endswith(".ipynb"):
                 source, line_cell_map, errors = read_notebook(file.read())
+                read_nb = True
             else:
                 source = file.read()
                 line_cell_map = []
                 errors = []
+                read_py = True
         if check_method_calls and METHODS:
             tree = annotate_ast.annotate_source(source, ast, PYTYPE_OPTIONS)
         else:
@@ -526,7 +533,7 @@ def check_file(
     except UnicodeError as error:
         print(f"{filename}: UNICODE ERROR: {error}")
     except json.decoder.JSONDecodeError as error:
-        print(f"{filename}:{error.lineno}: FORMAT ERROR: {error.msg}")
+        print(f"{filename}:{error.lineno}: FORMAT ERROR: invalid JSON")
     except ValueError as error:
         print(f"{filename}: VALUE ERROR: {error}")
     except annotate_ast.PytypeError as error:
@@ -540,7 +547,7 @@ def check_file(
             print(f"{filename}: PYTYPE ERROR: {message}")
 
 
-SYNTAX_MSG = "SYNTAX ERROR: this cell has not been checked"
+SYNTAX_MSG = "SYNTAX ERROR: this cell wasn't checked"
 
 
 def read_notebook(file_contents: str) -> tuple[str, list, list]:
@@ -577,7 +584,7 @@ def read_notebook(file_contents: str) -> tuple[str, list, list]:
 
 def main() -> None:
     """Implement the CLI."""
-    global FILE_UNIT, LANGUAGE, IMPORTS, METHODS
+    global FILE_UNIT, LANGUAGE, IMPORTS, METHODS, read_nb, read_py
 
     argparser = argparse.ArgumentParser(
         prog="allowed",
@@ -607,29 +614,29 @@ def main() -> None:
         "--unit",
         type=int,
         default=0,
-        help="only use constructs from units 1 to UNIT (default: all units)",
+        help="only allow constructs from units 1 to UNIT (default: all units)",
     )
     argparser.add_argument(
         "-c",
         "--config",
         default="m269.json",
-        help="configuration file (default: m269.json)",
+        help="allow the constructs given in CONFIG (default: m269.json)",
     )
     argparser.add_argument("file_or_folder", nargs="+", help="file or folder to check")
     args = argparser.parse_args()
 
     if args.methods and not PYTYPE_INSTALLED:
-        sys.exit("error: can't check method calls (need pytype)")
+        sys.exit("ERROR: can't check method calls (pytype not installed)")
     if args.unit < 0:
-        sys.exit("error: unit must be positive")
+        sys.exit("ERROR: unit must be positive")
 
     for file in (Path(args.config), Path(__file__).parent / args.config):
         if file.exists():
             with file.open() as config_file:
                 configuration = json.load(config_file)
             break
     else:
-        sys.exit(f"error: configuration file {args.config} not found")
+        sys.exit(f"ERROR: configuration file {args.config} not found")
     FILE_UNIT = configuration.get("FILE_UNIT", "")
     LANGUAGE = {}
     for key, value in configuration["LANGUAGE"].items():
@@ -652,13 +659,14 @@ def main() -> None:
             unit = args.unit if args.unit else get_unit(Path(name).name)
             check_file(name, get_constructs(unit), args.methods, args.first)
         else:
-            print(f"{name}: not a folder, Python file or notebook")
-
-    if not args.methods:
-        print("WARNING: didn't check method calls (use option -m)")
-    if not IPYTHON_INSTALLED:
-        print("WARNING: didn't check notebook cells with magics (need IPython)")
-    if args.first:
+            print(f"WARNING: {name} skipped: not a folder, Python file or notebook")
+
+    if (read_py or read_nb) and not args.methods:
+        print("WARNING: didn't check method calls",
+              "(use option -m)" if PYTYPE_INSTALLED else "(pytype not installed)")
+    if read_nb and not IPYTHON_INSTALLED:
+        print("WARNING: didn't check notebook cells with %-commands (IPython not installed)")
+    if (read_py or read_nb) and args.first:
         print("WARNING: each construct was reported once; other occurrences may exist")
 
 if __name__ == "__main__":

pyproject.toml

@@ -1,6 +1,6 @@
 [tool.poetry]
 name = "allowed"
-version = "1.3dev7" # as in allowed.py
+version = "1.3dev8" # as in allowed.py
 description = "Check if a program only uses a subset of the Python language."
 authors = ["Michel Wermelinger <michel.wermelinger@open.ac.uk>"]
 readme = "README.md"

tests/folder-first.txt

@@ -1,4 +1,4 @@
-tests/sample.ipynb:cell_1:2: SYNTAX ERROR: this cell has not been checked
+tests/sample.ipynb:cell_1:2: SYNTAX ERROR: this cell wasn't checked
 tests/sample.ipynb:cell_2:4: types
 tests/sample.ipynb:cell_2:5: choice
 tests/sample.ipynb:cell_2:10: assert
@@ -30,11 +30,11 @@ allowed/allowed.py:275: f-string
 allowed/allowed.py:287: :=
 allowed/allowed.py:288: int()
 allowed/allowed.py:384: hasattr()
-allowed/allowed.py:403: isinstance()
-allowed/allowed.py:408: type()
-allowed/allowed.py:498: with
-allowed/allowed.py:580: global
-allowed/allowed.py:630: break
-allowed/allowed.py:631: for-else
+allowed/allowed.py:406: isinstance()
+allowed/allowed.py:411: type()
+allowed/allowed.py:500: global
+allowed/allowed.py:503: with
+allowed/allowed.py:637: break
+allowed/allowed.py:638: for-else
 WARNING: didn't check method calls (use option -m)
 WARNING: each construct was reported once; other occurrences may exist

tests/foobar-fm.txt

@@ -0,0 +1 @@
+WARNING: foobar skipped: not a folder, Python file or notebook

tests/foobar-hfm.txt

@@ -0,0 +1,20 @@
+usage: allowed [-h] [-V] [-f] [-m] [-u UNIT] [-c CONFIG]
+               file_or_folder [file_or_folder ...]
+
+Check that the code only uses certain constructs. See http://dsa-
+ou.github.io/allowed for how to specify the constructs.
+
+positional arguments:
+  file_or_folder        file or folder to check
+
+options:
+  -h, --help            show this help message and exit
+  -V, --version         show program's version number and exit
+  -f, --first           report only the first of each disallowed construct
+                        (per file)
+  -m, --methods         enable method call checking
+  -u UNIT, --unit UNIT  only allow constructs from units 1 to UNIT (default:
+                        all units)
+  -c CONFIG, --config CONFIG
+                        allow the constructs given in CONFIG (default:
+                        m269.json)

tests/sample-nb-m.txt

@@ -1,4 +1,4 @@
-tests/sample.ipynb:cell_1:2: SYNTAX ERROR: this cell has not been checked
+tests/sample.ipynb:cell_1:2: SYNTAX ERROR: this cell wasn't checked
 tests/sample.ipynb:cell_2:4: types
 tests/sample.ipynb:cell_2:5: choice
 tests/sample.ipynb:cell_2:10: assert

tests/sample-nb-none.txt

@@ -0,0 +1,16 @@
+10,15c10,17
+< tests/sample.ipynb:cell_3:1: SYNTAX ERROR: this cell wasn't checked
+< tests/sample.ipynb:cell_4:1: SYNTAX ERROR: this cell wasn't checked
+< tests/sample.ipynb:cell_5:17: SYNTAX ERROR: this cell wasn't checked
+< tests/sample.ipynb:cell_6:3: SYNTAX ERROR: this cell wasn't checked
+< WARNING: didn't check method calls (pytype not installed)
+< WARNING: didn't check notebook cells with %-commands (IPython not installed)
+---
+> tests/sample.ipynb:cell_5:7: break
+> tests/sample.ipynb:cell_5:12: continue
+> tests/sample.ipynb:cell_5:13: while-else
+> tests/sample.ipynb:cell_6:4: f-string
+> tests/sample.ipynb:cell_6:9: <<
+> tests/sample.ipynb:cell_6:10: math.e
+> tests/sample.ipynb:cell_6:11: type()
+> WARNING: didn't check method calls (use option -m)

tests/sample-nb.txt

@@ -1,4 +1,4 @@
-tests/sample.ipynb:cell_1:2: SYNTAX ERROR: this cell has not been checked
+tests/sample.ipynb:cell_1:2: SYNTAX ERROR: this cell wasn't checked
 tests/sample.ipynb:cell_2:4: types
 tests/sample.ipynb:cell_2:5: choice
 tests/sample.ipynb:cell_2:10: assert

tests/sample-py-none.txt

@@ -0,0 +1,4 @@
+16c16
+< WARNING: didn't check method calls (pytype not installed)
+---
+> WARNING: didn't check method calls (use option -m)

tests/tests.sh

@@ -5,7 +5,13 @@ cmd='python allowed/allowed.py'
 if [ $# -eq 0 ]; then
     echo "Usage: ./tests.sh [run|create]"
 elif [ $1 = "run" ]; then
-    echo "sample.py"; echo "---"
+    # test that there are no warnings if no file is processed
+    echo "foobar -fm"; echo "---"
+    $cmd foobar -fm | diff -w - tests/foobar-fm.txt
+    # -h trumps other flags
+    echo; echo "foobar -hfm"; echo "---"
+    $cmd foobar -hfm | diff -w - tests/foobar-hfm.txt
+    echo; echo "sample.py"; echo "---"
     $cmd tests/sample.py | diff -w - tests/sample-py.txt
     echo ; echo "sample.py -m"; echo "---"
     $cmd tests/sample.py -m | diff -w - tests/sample-py-m.txt
@@ -16,6 +22,8 @@ elif [ $1 = "run" ]; then
     echo; echo "-f sample.ipynb allowed/"; echo "---"
     $cmd -f tests/sample.ipynb allowed | diff -w - tests/folder-first.txt
 elif [ $1 = "create" ]; then
+    $cmd foobar -fm > tests/foobar-fm.txt
+    $cmd foobar -hfm > tests/foobar-hfm.txt
     $cmd tests/sample.py > tests/sample-py.txt
     $cmd tests/sample.py -m > tests/sample-py-m.txt
     $cmd tests/sample.ipynb > tests/sample-nb.txt