Run repo checks entirely with Github Actions

[FichteFoll]

We don't need to go through PC server, which may have issues and occasionally produces unexpected 500 errors that we are unable to debug. Instead, we should be able to perform the necessary checks through Github Actions entirely. That way we are independent of the PC server, don't consume API calls of the rate limit (and I can apply fixes myself instead of Will being the only one who can change things).

References:

• sublimelsp/repository@158ef61
• sublimelsp/repository@7ba3a32

[rchl]

It had to be refactored a bit because running workflows from PRs created from forked repositories (so basically the usual case in this repo) didn't work correctly. In those cases, GitHub doesn't pass the access token to the workflow so the review code was unable to post review comments.

The solution to that is using a pull_request_target event rather than pull_request and separating the test code and actions into their own repos. This allows running the test code from actions rather than the pull request branch itself and mitigates potential security issues.

Coincidentally it also makes it easier to set up. There are now two actions:

• https://github.com/sublimelsp/st-package-reviewer-action
• https://github.com/sublimelsp/st-schema-reviewer-action

An example of how to trigger those from the workflow is at https://github.com/sublimelsp/st-package-reviewer-action

[deathaxe]

It appears some efforts were made to refactor package reviewer, so it can run as Github Action.

Would it make sense to move the action to a more common organization, which could probably also be the target to collect the other parts/repos at some point?

We could organize everything under SublimeText or packagecontrol org.

Thoughts?

[rchl]

I agree the code of those actions should be moved.

Just a small note that I've recalled when looking at the code yesterday...

There is this part in the reviewer (https://github.com/sublimelsp/st-package-reviewer-action/blob/e98168e07368ac1cb0f4d0c790b273195d9b8f89/lib/run_repo_tests.py#L175-L181) that I've disabled because apparently the original reviewer code on github is not up to date with what ST repo runs.

[FichteFoll]

Thoughts?

packagecontrol org is definitely my proposal, next to the package reviewer itself.

[deathaxe]

I think so, too, especially with the idea in mind to also move all the other package_control related repos over there. So everything would be at one place.

@rchl: Would you initiate repo transfer?

@FichteFoll: Could you add us as maintainer?

[rchl]

I can do that.
If someone can also get a hold of Will and tell him to push latest changes to https://github.com/packagecontrol/st_package_reviewer then I could update the action to be in sync with what the current reviewer's code.

[deathaxe]

That's why I asked Fichte to add us as member/maintainer. He seems to be the org admin there.

I guess, those actions need some updates to support PC 4.0.0 schemes in future as well.

[rchl]

That's why I asked Fichte to add us as member/maintainer. He seems to be the org admin there.

What are you referring to now? I meant that probably only wbond has the missing code in his local repo (or maybe on the PC server). So I don't think Fichte can help with that.

[deathaxe]

Well, maybe I am missing something, but it appears to me st_package_reviewer is mainly authored and mainteined by Fichte on github.com/packagecontrol org and st-package-reviewer-action installing it via pip to make use of it.

The action also vendors latest package_control 3.4.1

So, which local changes do you expect to be missing?

[rchl]

It's been a while since I've looked into it but I believe it's about those lines that I've commented out:

                if checker == CheckMessages:
                    for release_source in spec['releases']:
                        if isinstance(release_source.get('tags'), str):
                            checker_obj.add_prefix(release_source.get('tags'))
                elif checker == CheckHasSublimeSyntax:
                    checker_obj.set_selector(info['releases'][0]['sublime_text'])

The checker_obj is missing add_prefix and set_selector methods in https://github.com/packagecontrol/st_package_reviewer - no references when searching: https://github.com/search?q=repo%3Apackagecontrol%2Fst_package_reviewer%20set_selector&type=code

[FichteFoll]

Could you add us as maintainer?

Done (for the org). (I thought @deathaxe was already an admin there tbh.)

---

Regarding the "missing code": That's some code that Will added for the packagecontrol.io site specifically. You should be able to find it in the repo. I think I reviewed/monitored that at some point, but it's been many years since then and I don't remember what exactly he added outside of submitting the review comment via the bot account.

Keep in mind that the version of st_package_reviewer inside the packagecontrol.io repo is based on an older version and does not have the newer AST checks, for example. I'm also unsure what the current state of the st_package_reviwer repo is, to be honest, because I know I had plans for it (and some actual TODO items, like re-considering many of the AST checks that I haven't really tested on packages in the wild and accepting a "min version" parameter based on the sublime_text version selector used in the channel so that some checks are automatically disabled or ignored, like the sublime-syntax vs tmLanguage check).

[rchl]

The state is a bit confusing indeed. I've compared those two repos (the st_package_reviewer part of those repos specifically).

Left: /packagecontrol.io/app/lib/st_package_reviewer
Right: /st_package_reviewer/st_package_reviewer

--- check/file/ast/__init__.py	2020-09-19 21:56:43.000000000 
+++ check/file/ast/__init__.py	2023-10-12 21:20:56.000000000 
@@ -1,11 +1,11 @@
 import functools
 import ast
 from pathlib import Path
-from st_package_reviewer.check.file import FileChecker
-from st_package_reviewer.check import find_all
+from ....check.file import FileChecker
+from ....check import find_all
 
 __all__ = ('AstChecker', 'get_checkers')
 
 
 class AstChecker(FileChecker, ast.NodeVisitor):
     """Groups checks for python source code."""
@@ -34,14 +34,14 @@
 
         with path.open("r") as f:
             try:
                 # Cast path to string for <py36
                 the_ast = ast.parse(f.read(), str(path))
             except SyntaxError as e:
-                self.fail("Unable to parse Python file (column {})".format(e.offset + 1),
-                          exception=e)
+                with self.context("Line: {}".format(e.lineno)):
+                    self.fail("Unable to parse Python file", exception=e)
             else:
                 self._ast_cache[path] = the_ast
                 return the_ast
 
     def node_context(self, node):
         return self.context("Line: {}, Column: {}".format(node.lineno, node.col_offset + 1))
--- check/file/ast/check_initialized_api.py	2020-09-19 21:56:43.000000000 
+++ check/file/ast/check_initialized_api.py	2023-10-12 21:19:54.000000000 
@@ -19,13 +19,12 @@
             if base.id in interesting:
                 return True
     return False
 
 
 class CheckInitializedApiUsage(AstChecker):
-
     """Check for function calls of the sublime module in unsafe places.
 
     Notably, these are:
 
     - the global module scope
     - __init__ methods of event listeners
@@ -81,13 +80,13 @@
 
         # All 'remaining' calls of `sublime` attributes that are not safe
         # must be in places where the API may not have been initialized.
         try:
             attr = node.func.attr
             id_ = node.func.value.id
-        except AttributeError as e:
+        except AttributeError:
             return
         else:
             if id_ == "sublime" and attr not in ("platform", "arch", "version", "channel"):
                 with self.node_context(node):
                     self.fail("Calling unsafe method {!r} of sublime module"
                               " when API may not have been initialized".format(attr))
--- check/file/ast/check_no_modify_sys_path.py	2020-09-19 21:56:43.000000000 
+++ check/file/ast/check_no_modify_sys_path.py	2023-10-12 21:19:56.000000000 
@@ -16,8 +16,8 @@
             if (
                 func.attr in {'append', 'insert'}
                 and func.value.attr == 'path'
                 and func.value.value.id == 'sys'
             ):
                 self._warn_about_modify_sys_path(node)
-        except Exception as e:
+        except Exception:
             return
--- check/file/ast/check_os_system_calls.py	2020-09-19 21:56:43.000000000 
+++ check/file/ast/check_os_system_calls.py	2023-10-12 21:19:57.000000000 
@@ -10,10 +10,10 @@
                   "Also make sure you thought about the platform key in your pull request.")
 
     def visit_Call(self, node):
         try:
             attr = node.func.attr
             id = node.func.value.id
-        except Exception as e:
+        except Exception:
             return
         if id == "os" and attr == "system":
             self._warn_about_os_system(node)
--- check/file/ast/check_platform_usage.py	2020-09-19 21:56:43.000000000 
+++ check/file/ast/check_platform_usage.py	2023-10-12 21:19:59.000000000 
@@ -27,10 +27,10 @@
             self._warn_platform_module_usage(node)
 
     def visit_Call(self, node):
         try:
             attr = node.func.attr
             id_ = node.func.value.id
-        except Exception as e:
+        except Exception:
             return
         if id_ == "sublime" and attr in ("platform", "arch"):
             self._warn_sublime_platform_usage(node)
--- check/file/check_keymaps.py	2020-09-19 21:56:43.000000000 
+++ check/file/check_keymaps.py	2023-10-12 21:20:01.000000000 
@@ -83,12 +83,18 @@
 
                 supplementary_keys = keys - allowed_keys
                 if supplementary_keys:
                     self.warn("Binding defines supplementary keys {}".format(supplementary_keys))
 
                 if 'keys' in binding:
+                    if binding['keys'] == ["<character>"]:
+                        if not binding.get('context'):
+                            self.fail("'<character>' bindings must have a 'context'")
+                            idx_to_del.add(i)
+                        continue
+
                     try:
                         norm_chords = k_map._verify_and_normalize_chords(binding['keys'])
                     except KeyMappingError as e:
                         self.fail(e.args[0])
                         idx_to_del.add(i)
                     else:
@@ -108,13 +114,12 @@
 class KeyMapping:
 
     _def_maps = None
 
     @classmethod
     def default_maps(cls):
-        # type: Dict[str, KeyMapping]
         if not cls._def_maps:
             cls._def_maps = {plat: cls(DATA_PATH / fname)
                              for plat, fname in zip(PLATFORMS, PLATFORM_FILENAMES)}
             # Verify and normalize default maps
             for k_map in cls._def_maps.values():
                 k_map._verify()
@@ -142,41 +147,48 @@
     def _verify(self):
         for binding in self.data:
             binding['keys'] = self._verify_and_normalize_chords(binding['keys'])
 
     @classmethod
     def _verify_and_normalize_chords(cls, chords):
-        modifiers = ("ctrl", "super", "alt", "shift", "primary")
-
         if not chords or not isinstance(chords, list):
             raise KeyMappingError("'keys' key is empty or not a list")
         norm_chords = []
         for key_chord in chords:
             if len(key_chord) == 1:
                 # Any single character key is valid (representing a symbol)
+                norm_chords.append(key_chord)
+                continue
+
+            elif key_chord in cls.MODIFIERS:
+                # Legal since ST4
+                if len(chords) == 1:
+                    # But we disallow
+                    # TODO report minimum version
+                    raise KeyMappingError("Single-chord modifier bindings are disallowed")
                 norm_chords.append(key_chord)
                 continue
 
             chord_parts = []
             while True:
                 key, plus, key_chord = key_chord.partition("+")
                 if not key_chord:  # we're at the end
                     if plus:  # a chord with '+' as key
                         key = plus
                     if not cls._key_is_valid(key):
                         raise KeyMappingError("Invalid key '{}'".format(key))
-                    chord_parts.sort(key=modifiers.index)
+                    chord_parts.sort(key=cls.MODIFIERS.index)
                     chord_parts.append(key)
                     break
 
                 if key == "option":
                     key = "alt"
                 elif key == "command":
                     key = "super"
                 # TODO "primary"
-                if key not in modifiers:
+                if key not in cls.MODIFIERS:
                     raise KeyMappingError("Invalid modifier key '{}'".format(key))
 
                 chord_parts.append(key)
 
             norm_chords.append("+".join(chord_parts))
 
@@ -211,6 +223,8 @@
                     "clear", "sysreq",
                     # these have single-character equivalents
                     # TODO resolve these aliases
                     "plus", "minus", "equals", "forward_slash", "backquote",
                     # Note: this list is incomplete and sourced from the default bindings
                     }
+
+    MODIFIERS = ("ctrl", "super", "alt", "altgr", "shift", "primary")
--- check/file/check_license.py	2020-09-19 21:56:43.000000000 
+++ check/file/check_license.py	2023-10-12 21:18:51.000000000 
@@ -5,12 +5,12 @@
 
 class CheckLicense(FileChecker):
 
     def check(self):
         has_license = any(
             True for p in self.base_path.iterdir()
-            if re.search(r'(?i)^license', p.name)
+            if re.search(r'(?i)^(un)?license', p.name)
         )
 
         if not has_license:
             self.warn("The package does not contain a top-level LICENSE file."
                       " A license helps users to contribute to the package.")
--- check/file/check_messages.py	2020-09-19 21:56:43.000000000 
+++ check/file/check_messages.py	2023-10-12 21:20:56.000000000 
@@ -1,27 +1,16 @@
 import json
-import re
 
 from ...lib.semver import SemVer
 
 from . import FileChecker
 
 
 class CheckMessages(FileChecker):
 
-    prefixes = None
-
-    def add_prefix(self, prefix):
-        if self.prefixes is None:
-            self.prefixes = {'v'}
-        self.prefixes.add(prefix)
-
     def check(self):
-        if self.prefixes is None:
-            self.prefixes = {'v'}
-
         msg_path = self.sub_path("messages.json")
         folder_exists = self.sub_path("messages").is_dir()
         file_exists = msg_path.is_file()
 
         if not (folder_exists or file_exists):
             return
@@ -35,20 +24,19 @@
                 try:
                     data = json.load(f)
                 except ValueError as e:
                     self.fail("unable to load `messages.json`", exception=e)
                     return
 
-            prefix_regex = '^(' + '|'.join(list(self.prefixes)) + ')'
             for key, rel_path in data.items():
                 if key == "install":
                     pass
-                elif SemVer.valid(re.sub(prefix_regex, '', key)):
+                elif SemVer.valid(key):
                     pass
                 else:
                     self.fail("Key {!r} is not 'install' or a valid semantic version"
                               .format(key))
 
                 messsage_path = self.sub_path(rel_path)
                 if not messsage_path.is_file():
                     self.fail("File '{}', as specified by key {!r}, does not exist"
                               .format(rel_path, key))
--- check/file/check_resource_file_validity.py	2020-09-19 21:56:43.000000000 
+++ check/file/check_resource_file_validity.py	2023-10-12 21:20:56.000000000 
@@ -1,7 +1,6 @@
-import sys
 import plistlib
 import xml.etree.ElementTree as ET
 from xml.parsers.expat import ExpatError
 
 from . import FileChecker
 from ...lib import jsonc
@@ -47,16 +46,13 @@
         }
 
         for file_path in self.globs(*plist_file_globs):
             with self.file_context(file_path):
                 with file_path.open('rb') as f:
                     try:
-                        if sys.version_info < (3, 4):
-                            plistlib.readPlist(f)
-                        else:
-                            plistlib.load(f)
+                        plistlib.load(f)
                     except (ValueError, ExpatError) as e:
                         self.fail("Invalid Plist", exception=e)
 
 
 class CheckXmlFiles(FileChecker):
 
--- check/file/check_resource_files.py	2020-09-19 21:56:43.000000000 
+++ check/file/check_resource_files.py	2023-10-12 21:20:56.000000000 
@@ -1,8 +1,7 @@
 import logging
-import re
 
 from . import FileChecker
 
 
 l = logging.getLogger(__name__)
 
@@ -23,17 +22,20 @@
                           .format(len(python_files_in_package)))
 
 
 class CheckHasResourceFiles(FileChecker):
 
     def check(self):
+        # Files with a hidden extension are excluded,
+        # as they serve no purpose without another file using them
+        # (e.g. a plugin).
         resource_file_globs = {
             "*.py",
             "**/*.sublime-build",
             "**/*.sublime-color-scheme",
-            "**/*.hidden-color-scheme",
+            # "**/*.hidden-color-scheme",
             "**/*.sublime-commands",
             "**/*.sublime-completions",
             "**/*.sublime-keymap",
             "**/*.sublime-macro",  # almost useless without other files
             "**/*.sublime-menu",
             "**/*.sublime-mousemap",
@@ -42,71 +44,30 @@
             "**/*.sublime-syntax",
             "**/*.sublime-theme",
             "**/*.tmLanguage",
             "**/*.tmPreferences",
             "**/*.tmSnippet",
             "**/*.tmTheme",
-            "**/*.hidden-tmTheme",
+            # "**/*.hidden-tmTheme",
             # hunspell dictionaries
             "**/*.aff",
             "**/*.dic",
         }
 
         has_resource_files = any(self.glob(ptrn) for ptrn in resource_file_globs)
         if not has_resource_files:
             self.fail("The package does not define any file that interfaces with Sublime Text")
 
 
 class CheckHasSublimeSyntax(FileChecker):
 
-    selector = None
-
-    def set_selector(self, selector):
-        self.selector = selector
-
-    def st_build_match(self, ver):
-        min_version = float("-inf")
-        max_version = float("inf")
-
-        if self.selector == '*':
-            return True
-
-        gt_match = re.match('>(\d+)$', self.selector)
-        ge_match = re.match('>=(\d+)$', self.selector)
-        lt_match = re.match('<(\d+)$', self.selector)
-        le_match = re.match('<=(\d+)$', self.selector)
-        range_match = re.match('(\d+) - (\d+)$', self.selector)
-
-        if gt_match:
-            min_version = int(gt_match.group(1)) + 1
-        elif ge_match:
-            min_version = int(ge_match.group(1))
-        elif lt_match:
-            max_version = int(lt_match.group(1)) - 1
-        elif le_match:
-            max_version = int(le_match.group(1))
-        elif range_match:
-            min_version = int(range_match.group(1))
-            max_version = int(range_match.group(2))
-        else:
-            return None
-
-        if min_version > ver:
-            return False
-        if max_version < ver:
-            return False
-
-        return True
-
     def check(self):
         syntax_files = self.glob("**/*.sublime-syntax")
 
         for path in syntax_files:
             if (
                 not path.with_suffix(".tmLanguage").is_file()
                 and not path.with_suffix(".hidden-tmLanguage").is_file()
             ):
-                if not self.st_build_match(3091) and not self.st_build_match(2221):
-                    continue
                 with self.file_context(path):
                     self.warn("'.sublime-syntax' support has been added in build 3092 and there "
                               "is no '.tmLanguage' fallback file")
--- check/__init__.py	2020-09-19 21:56:43.000000000 
+++ check/__init__.py	2023-10-12 21:20:56.000000000 
@@ -76,20 +76,13 @@
         rel_path = checker_file.relative_to(path)
         if rel_path.name == "__init__.py":
             l.debug("Skipping %s", rel_path)
             continue
 
         l.debug("Loading %s...", rel_path)
-        # The Python 3.3 shim for pathlib doesn't support: .with_suffix('')
-        if sys.version_info < (3, 4):
-            rel_path_no_suffix = str(rel_path)
-            if rel_path_no_suffix.endswith('.py'):
-                rel_path_no_suffix = rel_path_no_suffix[:-3]
-        else:
-            rel_path_no_suffix = str(rel_path.with_suffix(''))
-        relative_module_segments = rel_path_no_suffix.split(os.sep)
+        relative_module_segments = str(rel_path.with_suffix('')).split(os.sep)
         module_path = "." + ".".join(relative_module_segments)
         module = importlib.import_module(module_path, package)
 
         for thing in module.__dict__.values():
             if not isinstance(thing, type):  # not a class
                 continue
--- 
+++ lib/__init__.py	2023-10-12 21:20:56.000000000 
@@ -0,0 +1 @@
+"""Package for third-party modules that aren't bundled on pypi."""

Besides that, the https://github.com/packagecontrol/st_package_reviewer/tree/master/st_package_reviewer/check/repo contains a repo directory which does not exist in https://github.com/wbond/packagecontrol.io/tree/master/app/lib/st_package_reviewer.

The https://github.com/packagecontrol/st_package_reviewer seems to have mostly newer changes but a notable thing that is missing in it are set_selector and add_prefix methods.

I think I mostly know how to handle it so I could pick and choose the correct changes myself in the action's repo but it would also be nice if https://github.com/wbond/packagecontrol.io repo would get updated with newer changes.

[rchl]

I've transferred both repos now:

• https://github.com/packagecontrol/st-package-reviewer-action
• https://github.com/packagecontrol/st-schema-reviewer-action

Two notes though:

• I think that the base-sha value that is passed to the st-package-reviewer-action action might not be optimal. I'm not entirely sure now but I think I saw cases where the range checked in the PR did not include the most recent merges from main.
• the st-package-reviewer-action action doesn't support http_basic_auth referenced in the original packagecontrol code here https://github.com/wbond/packagecontrol.io/blob/4b146667805cb0ef4c0da1b4be09bff6f30f9d85/config/crawler.yml#L12-L15 . Not sure if that's really required or it's just to avoid API being throttled...