Maplint tool now has proper github action error messages (#72920)

The tool added in #72372 is pretty awesome. The output is uhh cryptic
though. I had to read the source code to realize the (line 382) or
whatever part of the message was the dmm line number and there's stack
traces everywhere. I've made it support github action error messages so
now you get this beauty if you mess up:

![Example cable
error](https://user-images.githubusercontent.com/1185434/214156870-d73ffba0-f79a-43ed-9574-e74cc2ee2057.png)

Or, in the run summary:

![image](https://user-images.githubusercontent.com/1185434/214157201-e392a6d6-a8a8-4d8a-ac74-c65ae97438c8.png)

Errors parsing the lint yml's will also output github action errors,
although the line number will always be 1 since the yaml parser discards
line numbers to my knowledge.

In the midst of doing this, I made the error type contain the file and
line info, and added a bunch of type hints in the midst of trying to
understand Mothblock's code.

Note that for power users, the default behavior is still colored
terminal text; `--github` is added by the CI suite to enable this
behavior.

Much easier to see where the errors are and what they are (who even
knows what a 'pop' is? The tg game code calls them grid models.)

Nothing player-facing.

.github/workflows/ci_suite.yml

@@ -41,7 +41,7 @@ jobs:
           tools/bootstrap/python -c ''
       - name: Run Linters
         run: |
-          tools/bootstrap/python -m tools.maplint.source
+          tools/bootstrap/python -m tools.maplint.source --github
           tools/build/build --ci lint tgui-test
           bash tools/ci/check_filedirs.sh shiptest.dme
           bash tools/ci/check_changelogs.sh

tools/maplint/source/__main__.py

@@ -13,31 +13,49 @@ def green(text):
 def red(text):
     return "\033[31m" + str(text) + "\033[0m"
 
-def process_dmm(map_filename, lints):
-    problems = []
+def process_dmm(map_filename, lints: dict[str, lint.Lint]) -> list[MaplintError]:
+    problems: list[MaplintError] = []
 
     with open(map_filename, "r") as file:
         try:
             map_data = dmm.parse_dmm(file)
         except MaplintError as error:
-            problems.append(red(f"Error parsing map.\n  {error}") + traceback.format_exc())
+            problems.append(error)
+            # No structured data to lint.
+            return problems
 
         for lint_name, lint in lints.items():
             try:
-                for result in lint.run(map_data):
-                    tail = f"\n  {lint.help}" if lint.help is not None else ""
-                    problems.append(f"{red(lint_name)}: {result}{tail}")
+                problems.extend(lint.run(map_data))
             except KeyboardInterrupt:
                 raise
             except Exception:
-                problems.append(f"{red('An exception occurred, this is either a bug in maplint or a bug in a lint.')}\n{traceback.format_exc()}")
+                problems.append(MaplintError(
+                    f"An exception occurred, this is either a bug in maplint or a bug in a lint. \n{traceback.format_exc()}",
+                    lint_name,
+                ))
 
     return problems
 
+def print_error(message: str, filename: str, line_number: int, github_error_style: bool):
+    if github_error_style:
+        print(f"::error file={filename},line={line_number},title=DMM Linter::{message}")
+    else:
+        print(red(f"- Error parsing {filename} (line {line_number}): {message}"))
+
+def print_maplint_error(error: MaplintError, github_error_style: bool):
+    print_error(
+        f"{f'(in pop {error.pop_id}) ' if error.pop_id else ''}{f'(at {error.coordinates}) ' if error.coordinates else ''}{error}",
+        error.file_name,
+        error.line_number,
+        github_error_style,
+    )
+
 def main(args):
     any_failed = False
+    github_error_style = args.github
 
-    lints = {}
+    lints: dict[str, lint.Lint] = {}
 
     lint_base = pathlib.Path(__file__).parent.parent / "lints"
     lint_filenames = []
@@ -48,41 +66,44 @@ def main(args):
 
     for lint_filename in lint_filenames:
         try:
-            lints[lint_filename.stem] = lint.Lint(yaml.safe_load(lint_filename.read_text()))
+            lints[lint_filename] = lint.Lint(yaml.safe_load(lint_filename.read_text()))
         except MaplintError as error:
-            print(red(f"Error loading {lint_filename.stem}.\n  ") + str(error))
+            print_maplint_error(error, github_error_style)
             any_failed = True
         except Exception:
-            print(red(f"Error loading {lint_filename.stem}."))
+            print_error("Error loading lint file.", lint_filename, 1, github_error_style)
             traceback.print_exc()
             any_failed = True
 
     for map_filename in (args.maps or glob.glob("_maps/**/*.dmm", recursive = True)):
         print(map_filename, end = " ")
 
         success = True
-        message = []
+        all_failures: list[MaplintError] = []
 
         try:
             problems = process_dmm(map_filename, lints)
             if len(problems) > 0:
                 success = False
-                message += problems
+                all_failures.extend(problems)
         except KeyboardInterrupt:
             raise
         except Exception:
             success = False
 
-            message.append(f"{red('An exception occurred, this is either a bug in maplint or a bug in a lint.')}\n{traceback.format_exc()}")
+            all_failures.append(MaplintError(
+                f"An exception occurred, this is either a bug in maplint or a bug in a lint.' {traceback.format_exc()}",
+                map_filename,
+            ))
 
         if success:
             print(green("OK"))
         else:
             print(red("X"))
             any_failed = True
 
-        for line in message:
-            print(f"- {line}")
+        for failure in all_failures:
+            print_maplint_error(failure, github_error_style)
 
     if any_failed:
         exit(1)
@@ -95,6 +116,7 @@ def main(args):
 
     parser.add_argument("maps", nargs = "*")
     parser.add_argument("--lints", nargs = "*")
+    parser.add_argument("--github", action='store_true')
 
     args = parser.parse_args()
 

tools/maplint/source/common.py

@@ -1,7 +1,7 @@
 import re
 from dataclasses import dataclass
 
-from .error import MaplintError
+from .error import MapParseError
 
 REGEX_TYPEPATH = re.compile(r'^/[\w/]+$')
 
@@ -11,7 +11,7 @@ class Typepath:
 
     def __init__(self, path):
         if not REGEX_TYPEPATH.match(path):
-            raise MaplintError(f"Invalid typepath {path!r}.")
+            raise MapParseError(f"Invalid typepath {path!r}.")
 
         self.path = path
         self.segments = path.split('/')[1:]

tools/maplint/source/dmm.py

@@ -2,9 +2,10 @@
 # by virtue of being read-only.
 import re
 from dataclasses import dataclass, field
+from typing import IO
 
 from .common import Constant, Filename, Null, Typepath
-from .error import MaplintError
+from .error import MapParseError, MaplintError
 
 REGEX_POP_ID = re.compile(r'^"(?P<key>.+)" = \($')
 REGEX_POP_CONTENT_HEADER = re.compile(r'^(?P<path>[/\w]+?)(?P<end>[{,)])$')
@@ -14,6 +15,8 @@
 @dataclass
 class Content:
     path: Typepath
+    filename: str
+    starting_line: int
     var_edits: dict[str, Constant] = field(default_factory = dict)
 
 @dataclass
@@ -37,19 +40,22 @@ class DMMParser:
     dmm: DMM
     line = 0
 
-    def __init__(self, reader):
+    def __init__(self, reader: IO):
         self.dmm = DMM()
         self.reader = reader
 
     def parse(self):
         if "dmm2tgm" not in self.next_line():
-            raise MaplintError("Map isn't in TGM format. Consider using StrongDMM instead of Dream Maker.\n  Please also consider installing the map merge tools, found through Install.bat in the tools/hooks folder.")
+            self.raise_error("Map isn't in TGM format. Consider using StrongDMM instead of Dream Maker.\n  Please also consider installing the map merge tools, found through Install.bat in the tools/hooks folder.")
 
-        while self.parse_pop():
-            pass
+        try:
+            while self.parse_pop():
+                pass
 
-        while self.parse_row():
-            pass
+            while self.parse_row():
+                pass
+        except MapParseError as error:
+            raise self.raise_error(error)
 
         return self.dmm
 
@@ -79,7 +85,7 @@ def parse_pop(self):
             if content_match is None:
                 self.raise_error("Pop content didn't lead to a path")
 
-            content = Content(Typepath(content_match.group("path")))
+            content = Content(Typepath(content_match.group("path")), self.reader.name, self.line)
             contents.append(content)
 
             content_end = content_match.group("end")
@@ -172,7 +178,7 @@ def expect(self, condition, message):
             self.raise_error(message)
 
     def raise_error(self, message):
-        raise MaplintError(f"{message} (line {self.line})")
+        raise MaplintError(message, self.reader.name, self.line)
 
-def parse_dmm(reader):
+def parse_dmm(reader: IO):
     return DMMParser(reader).parse()

tools/maplint/source/error.py

@@ -1,2 +1,23 @@
+"""Linting error with associated filename and line number."""
 class MaplintError(Exception):
+    """The DMM file name the exception occurred in"""
+    file_name = "unknown"
+
+    """The line the error occurred on"""
+    line_number = 1
+
+    """The optional coordinates"""
+    coordinates: str = None
+
+    """The optional pop ID"""
+    pop_id: str = None
+
+    def __init__(self, message: str, file_name: str, line_number = 1):
+        Exception.__init__(self, message)
+
+        self.file_name = file_name
+        self.line_number = line_number
+
+"""A parsing error that must be upgrading to a linting error by parse()."""
+class MapParseError(Exception):
     pass

tools/maplint/source/lint.py

@@ -3,11 +3,15 @@
 
 from .common import Constant, Typepath
 from .dmm import DMM, Content
-from .error import MaplintError
+from .error import MaplintError, MapParseError
 
 def expect(condition, message):
     if not condition:
-        raise MaplintError(message)
+        raise MapParseError(message)
+
+"""Create an error linked to a specific content instance"""
+def fail_content(content: Content, message: str) -> MaplintError:
+    return MaplintError(message, content.filename, content.starting_line)
 
 class TypepathExtra:
     typepath: Typepath
@@ -99,9 +103,9 @@ def extract_choices(data, key) -> Optional[Choices]:
             pattern = constants_data.pop("pattern")
             return re.compile(pattern)
 
-        raise MaplintError(f"Unknown key in {key}: {', '.join(constants_data.keys())}.")
+        raise MapParseError(f"Unknown key in {key}: {', '.join(constants_data.keys())}.")
 
-    raise MaplintError(f"{key} must be a list of constants, or a pattern")
+    raise MapParseError(f"{key} must be a list of constants, or a pattern")
 
 class BannedVariable:
     variable: str
@@ -119,7 +123,7 @@ def __init__(self, variable, data = {}):
 
         expect(len(data) == 0, f"Unknown key in banned variable {variable}: {', '.join(data.keys())}.")
 
-    def run(self, identified: Content):
+    def run(self, identified: Content) -> str:
         if identified.var_edits[self.variable] is None:
             return None
 
@@ -179,30 +183,30 @@ def __init__(self, data):
 
         expect(len(data) == 0, f"Unknown lint rules: {', '.join(data.keys())}.")
 
-    def run(self, identified: Content, contents: list[Content], identified_index) -> list[str]:
-        failures = []
+    def run(self, identified: Content, contents: list[Content], identified_index) -> list[MaplintError]:
+        failures: list[MaplintError] = []
 
         if self.banned:
-            failures.append(f"Typepath {identified.path} is banned.")
+            failures.append(fail_content(identified, f"Typepath {identified.path} is banned."))
 
         for banned_neighbor in self.banned_neighbors:
             for neighbor in contents[:identified_index] + contents[identified_index + 1:]:
                 if not banned_neighbor.matches(identified, neighbor):
                     continue
 
-                failures.append(f"Typepath {identified.path} has a banned neighbor: {neighbor.path}")
+                failures.append(fail_content(identified, f"Typepath {identified.path} has a banned neighbor: {neighbor.path}"))
 
         if self.banned_variables == True:
             if len(identified.var_edits) > 0:
-                failures.append(f"Typepath {identified.path} should not have any variable edits.")
+                failures.append(fail_content(identified, f"Typepath {identified.path} should not have any variable edits."))
         else:
             assert isinstance(self.banned_variables, list)
             for banned_variable in self.banned_variables:
                 if banned_variable.variable in identified.var_edits:
                     ban_reason = banned_variable.run(identified)
                     if ban_reason is None:
                         continue
-                    failures.append(f"Typepath {identified.path} has a banned variable (set to {identified.var_edits[banned_variable.variable]}): {banned_variable.variable}. {ban_reason}")
+                    failures.append(fail_content(identified, f"Typepath {identified.path} has a banned variable (set to {identified.var_edits[banned_variable.variable]}): {banned_variable.variable}. {ban_reason}"))
 
         return failures
 
@@ -223,8 +227,8 @@ def __init__(self, data):
         for typepath, rules in data.items():
             self.rules[TypepathExtra(typepath)] = Rules(rules)
 
-    def run(self, map_data: DMM):
-        results = []
+    def run(self, map_data: DMM) -> list[MaplintError]:
+        all_failures: list[MaplintError] = []
         (width, height) = map_data.size()
 
         for pop, contents in map_data.pops.items():
@@ -256,6 +260,10 @@ def run(self, map_data: DMM):
                         coordinate_texts.append(f"and {leftover_coordinates} more")
 
                     for failure in failures:
-                        results.append(f"{failure}\n  Found at pop {pop} (found in {', '.join(coordinate_texts)})")
+                        if self.help is not None:
+                            failure.message += f"\n  {self.help}"
+                        failure.coordinates = ', '.join(coordinate_texts)
+                        failure.pop_id = pop
+                        all_failures.append(failure)
 
-        return list(set(results))
+        return list(set(all_failures))

tools/requirements.txt

@@ -1,6 +1,6 @@
 pygit2==1.7.2
 bidict==0.22.0
-Pillow==9.5.0
+Pillow==10.0.1
 
 # check_regex.py
 colorama==0.4.4