From e393b16909c5c2e698cd1f461040eebb1d5be839 Mon Sep 17 00:00:00 2001 From: Simon Cozens Date: Fri, 1 Sep 2023 08:54:45 +0100 Subject: [PATCH 1/5] Refactor - move URL into object so methods can post their own data --- Lib/gftools/qa.py | 11 +++++++---- Lib/gftools/scripts/qa.py | 19 +++++++++++-------- 2 files changed, 18 insertions(+), 12 deletions(-) diff --git a/Lib/gftools/qa.py b/Lib/gftools/qa.py index 1c61cf279..10f0dfbdf 100644 --- a/Lib/gftools/qa.py +++ b/Lib/gftools/qa.py @@ -16,10 +16,11 @@ class FontQA: - def __init__(self, fonts, fonts_before=None, out="out"): + def __init__(self, fonts, fonts_before=None, out="out", url=None): self.fonts = fonts self.fonts_before = fonts_before self.out = out + self.url = url def diffenator(self, **kwargs): logger.info("Running Diffenator") @@ -102,14 +103,16 @@ def render(self, imgs=False): else: self.proof(imgs) - def post_to_github(self, url): + def post_to_github(self): """Post Fontbakery report as a new issue or as a comment to an open PR""" + if not self.url: + return # Parse url tokens - url_split = url.split("/") + url_split = self.url.split("/") repo_owner = url_split[3] repo_name = url_split[4] - issue_number = url_split[-1] if "pull" in url else None + issue_number = url_split[-1] if "pull" in self.url else None fontbakery_report = os.path.join(self.out, "Fontbakery", "report.md") if not os.path.isfile(fontbakery_report): diff --git a/Lib/gftools/scripts/qa.py b/Lib/gftools/scripts/qa.py index eb0b0c140..27262e4f4 100755 --- a/Lib/gftools/scripts/qa.py +++ b/Lib/gftools/scripts/qa.py @@ -216,12 +216,20 @@ def main(args=None): family_name, fonts_before_dir ) + url = None + if args.out_url: + url = args.out_url + elif args.out_github and args.pull_request: + url = args.pull_request + elif args.out_github and args.github_dir: + url = args.github_dir + if fonts_before: dfonts_before = [DFont(f) for f in fonts_before if f.endswith((".ttf", ".otf")) and "static" not in f] - qa = FontQA(dfonts, dfonts_before, args.out) + qa = FontQA(dfonts, dfonts_before, args.out, url=url) else: - qa = FontQA(dfonts, out=args.out) + qa = FontQA(dfonts, out=args.out, url=url) if args.auto_qa and family_on_gf: qa.googlefonts_upgrade(args.imgs) @@ -238,12 +246,7 @@ def main(args=None): if args.proof: qa.proof() - if args.out_url: - qa.post_to_github(args.out_url) - elif args.out_github and args.pull_request: - qa.post_to_github(args.pull_request) - elif args.out_github and args.github_dir: - qa.post_to_github(args.github_dir) + qa.post_to_github() if __name__ == "__main__": From e7b4b70544688f2f072ae5b29479bf20f1106132 Mon Sep 17 00:00:00 2001 From: Simon Cozens Date: Fri, 1 Sep 2023 09:02:27 +0100 Subject: [PATCH 2/5] Temporary refactor - split posting FB reports from posting other kinds of content --- Lib/gftools/qa.py | 31 +++++++++++++++++-------------- 1 file changed, 17 insertions(+), 14 deletions(-) diff --git a/Lib/gftools/qa.py b/Lib/gftools/qa.py index 10f0dfbdf..8640263e8 100644 --- a/Lib/gftools/qa.py +++ b/Lib/gftools/qa.py @@ -104,7 +104,19 @@ def render(self, imgs=False): self.proof(imgs) def post_to_github(self): - """Post Fontbakery report as a new issue or as a comment to an open + """Post fontbakery report to GitHub""" + fontbakery_report = os.path.join(self.out, "Fontbakery", "report.md") + if not os.path.isfile(fontbakery_report): + logger.warning( + "Cannot Post Github message because no Fontbakery report exists" + ) + return + with open(fontbakery_report) as doc: + msg = doc.read() + self._post_to_github(msg) + + def _post_to_github(self, text): + """Post text as a new issue or as a comment to an open PR""" if not self.url: return @@ -114,18 +126,9 @@ def post_to_github(self): repo_name = url_split[4] issue_number = url_split[-1] if "pull" in self.url else None - fontbakery_report = os.path.join(self.out, "Fontbakery", "report.md") - if not os.path.isfile(fontbakery_report): - logger.warning( - "Cannot Post Github message because no Fontbakery report exists" - ) - return - client = GitHubClient(repo_owner, repo_name) - with open(fontbakery_report) as doc: - msg = doc.read() - if issue_number: - client.create_issue_comment(issue_number, msg) - else: - client.create_issue("Google Font QA report", msg) + if issue_number: + client.create_issue_comment(issue_number, text) + else: + client.create_issue("Google Font QA report", text) From 6e8def9a75aabab0237487ac331126a2b8f3383c Mon Sep 17 00:00:00 2001 From: Simon Cozens Date: Fri, 1 Sep 2023 09:04:25 +0100 Subject: [PATCH 3/5] The fontbakery method now posts its own report --- Lib/gftools/qa.py | 26 ++++++++++++-------------- Lib/gftools/scripts/qa.py | 2 -- 2 files changed, 12 insertions(+), 16 deletions(-) diff --git a/Lib/gftools/qa.py b/Lib/gftools/qa.py index 8640263e8..414a50634 100644 --- a/Lib/gftools/qa.py +++ b/Lib/gftools/qa.py @@ -40,7 +40,7 @@ def diffenator(self, **kwargs): diffenator=True, diffbrowsers=False, ) - + def diffbrowsers(self, imgs=False): logger.info("Running Diffbrowsers") if not self.fonts_before: @@ -88,6 +88,16 @@ def fontbakery(self, profile="googlefonts", html=False, extra_args=None): cmd.extend(extra_args) subprocess.call(cmd) + fontbakery_report = os.path.join(self.out, "Fontbakery", "report.md") + if not os.path.isfile(fontbakery_report): + logger.warning( + "Cannot Post Github message because no Fontbakery report exists" + ) + return + with open(fontbakery_report) as doc: + msg = doc.read() + self.post_to_github(msg) + def googlefonts_upgrade(self, imgs=False): self.fontbakery() self.diffenator() @@ -103,19 +113,7 @@ def render(self, imgs=False): else: self.proof(imgs) - def post_to_github(self): - """Post fontbakery report to GitHub""" - fontbakery_report = os.path.join(self.out, "Fontbakery", "report.md") - if not os.path.isfile(fontbakery_report): - logger.warning( - "Cannot Post Github message because no Fontbakery report exists" - ) - return - with open(fontbakery_report) as doc: - msg = doc.read() - self._post_to_github(msg) - - def _post_to_github(self, text): + def post_to_github(self, text): """Post text as a new issue or as a comment to an open PR""" if not self.url: diff --git a/Lib/gftools/scripts/qa.py b/Lib/gftools/scripts/qa.py index 27262e4f4..ee1b6fda7 100755 --- a/Lib/gftools/scripts/qa.py +++ b/Lib/gftools/scripts/qa.py @@ -246,8 +246,6 @@ def main(args=None): if args.proof: qa.proof() - qa.post_to_github() - if __name__ == "__main__": main() From 52c2ef2ffd0e2317f6e3516a5034d333e9247c34 Mon Sep 17 00:00:00 2001 From: Simon Cozens Date: Fri, 1 Sep 2023 09:04:35 +0100 Subject: [PATCH 4/5] =?UTF-8?q?Run=20black,=20while=20we=E2=80=99re=20here?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- Lib/gftools/qa.py | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/Lib/gftools/qa.py b/Lib/gftools/qa.py index 414a50634..2ac742230 100644 --- a/Lib/gftools/qa.py +++ b/Lib/gftools/qa.py @@ -4,12 +4,17 @@ from gftools.gfgithub import GitHubClient from gftools.utils import mkdir + try: from diffenator2 import ninja_diff, ninja_proof except ModuleNotFoundError: - raise ModuleNotFoundError(("gftools was installed without the QA " - "dependencies. To install the dependencies, see the ReadMe, " - "https://github.com/googlefonts/gftools#installation")) + raise ModuleNotFoundError( + ( + "gftools was installed without the QA " + "dependencies. To install the dependencies, see the ReadMe, " + "https://github.com/googlefonts/gftools#installation" + ) + ) logger = logging.getLogger(__name__) logger.setLevel(logging.INFO) @@ -25,9 +30,7 @@ def __init__(self, fonts, fonts_before=None, out="out", url=None): def diffenator(self, **kwargs): logger.info("Running Diffenator") if not self.fonts_before: - logger.warning( - "Cannot run Diffenator since there are no fonts before" - ) + logger.warning("Cannot run Diffenator since there are no fonts before") return dst = os.path.join(self.out, "Diffenator") ninja_diff( @@ -44,9 +47,7 @@ def diffenator(self, **kwargs): def diffbrowsers(self, imgs=False): logger.info("Running Diffbrowsers") if not self.fonts_before: - logger.warning( - "Cannot run diffbrowsers since there are no fonts before" - ) + logger.warning("Cannot run diffbrowsers since there are no fonts before") return dst = os.path.join(self.out, "Diffbrowsers") mkdir(dst) @@ -60,7 +61,7 @@ def diffbrowsers(self, imgs=False): diffenator=False, diffbrowsers=True, ) - + def proof(self, imgs=False): logger.info("Running proofing tools") dst = os.path.join(self.out, "Proof") @@ -77,7 +78,7 @@ def fontbakery(self, profile="googlefonts", html=False, extra_args=None): out = os.path.join(self.out, "Fontbakery") mkdir(out) cmd = ( - ["fontbakery", "check-"+profile, "-l", "INFO", "--succinct"] + ["fontbakery", "check-" + profile, "-l", "INFO", "--succinct"] + [f.path for f in self.fonts] + ["-C"] + ["--ghmarkdown", os.path.join(out, "report.md")] From 60f6ef16c054b9428415bb50be3541a00e24dcfb Mon Sep 17 00:00:00 2001 From: Simon Cozens Date: Fri, 1 Sep 2023 09:23:19 +0100 Subject: [PATCH 5/5] Wrap functions in decorator which catches exceptions and posts them to GH --- Lib/gftools/qa.py | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/Lib/gftools/qa.py b/Lib/gftools/qa.py index 2ac742230..7c34c29f7 100644 --- a/Lib/gftools/qa.py +++ b/Lib/gftools/qa.py @@ -1,6 +1,7 @@ import logging import os import subprocess +import traceback from gftools.gfgithub import GitHubClient from gftools.utils import mkdir @@ -20,6 +21,20 @@ logger.setLevel(logging.INFO) +def report_exceptions(meth): + def safe_call(self, *args, **kwargs): + try: + meth(self, *args, **kwargs) + except Exception as e: + msg = f"Call to {meth.__name__} failed:\n{e}" + print(msg) + print() + print(traceback.format_exc()) + self.post_to_github(msg+"\n\n"+"See CI logs for more details") + + return safe_call + + class FontQA: def __init__(self, fonts, fonts_before=None, out="out", url=None): self.fonts = fonts @@ -27,6 +42,7 @@ def __init__(self, fonts, fonts_before=None, out="out", url=None): self.out = out self.url = url + @report_exceptions def diffenator(self, **kwargs): logger.info("Running Diffenator") if not self.fonts_before: @@ -44,6 +60,7 @@ def diffenator(self, **kwargs): diffbrowsers=False, ) + @report_exceptions def diffbrowsers(self, imgs=False): logger.info("Running Diffbrowsers") if not self.fonts_before: @@ -62,6 +79,7 @@ def diffbrowsers(self, imgs=False): diffbrowsers=True, ) + @report_exceptions def proof(self, imgs=False): logger.info("Running proofing tools") dst = os.path.join(self.out, "Proof") @@ -73,6 +91,7 @@ def proof(self, imgs=False): filter_styles=None, ) + @report_exceptions def fontbakery(self, profile="googlefonts", html=False, extra_args=None): logger.info("Running Fontbakery") out = os.path.join(self.out, "Fontbakery")