[{"id":31851,"web_url":"https://patchwork.libcamera.org/comment/31851/","msgid":"<172951463225.3674438.282375606387170899@ping.linuxembedded.co.uk>","date":"2024-10-21T12:43:52","subject":"Re: [PATCH 1/4] utils: checkstyle.py: Factor out common code to new\n\tCheckerBase class","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Laurent Pinchart (2024-10-18 20:32:43)\n> The CommitChecker, StyleChecker and Formatter classes duplicate code.\n> Create a new CheckerBase class to factor out common code.\n> \n\nI'm not sure if I'm the best reviewer here, but this looks fine to me.\n\n\nReviewed-by: Kieran Bingham \n\n> Signed-off-by: Laurent Pinchart \n> ---\n> utils/checkstyle.py | 140 ++++++++++++++++----------------------------\n> 1 file changed, 51 insertions(+), 89 deletions(-)\n> \n> diff --git a/utils/checkstyle.py b/utils/checkstyle.py\n> index ab89c0a14fab..1de518953dcd 100755\n> --- a/utils/checkstyle.py\n> +++ b/utils/checkstyle.py\n> @@ -334,37 +334,57 @@ class Amendment(Commit):\n> class ClassRegistry(type):\n> def __new__(cls, clsname, bases, attrs):\n> newclass = super().__new__(cls, clsname, bases, attrs)\n> - if bases:\n> - bases[0].subclasses.append(newclass)\n> - bases[0].subclasses.sort(key=lambda x: getattr(x, 'priority', 0),\n> - reverse=True)\n> + if bases and bases[0] != CheckerBase:\n> + base = bases[0]\n> +\n> + if not hasattr(base, 'subclasses'):\n> + base.subclasses = []\n> + base.subclasses.append(newclass)\n> + base.subclasses.sort(key=lambda x: getattr(x, 'priority', 0),\n> + reverse=True)\n> return newclass\n> \n> \n> +class CheckerBase(metaclass=ClassRegistry):\n> + #\n> + # Class methods\n> + #\n> + @classmethod\n> + def instances(cls, obj, names):\n> + for instance in cls.subclasses:\n> + if names and instance.__name__ not in names:\n> + continue\n> + if instance.supports(obj):\n> + yield instance\n> +\n> + @classmethod\n> + def supports(cls, obj):\n> + if hasattr(cls, 'commit_types'):\n> + return type(obj) in cls.commit_types\n> +\n> + if hasattr(cls, 'patterns'):\n> + for pattern in cls.patterns:\n> + if fnmatch.fnmatch(os.path.basename(obj), pattern):\n> + return True\n> +\n> + return False\n> +\n> + @classmethod\n> + def all_patterns(cls):\n> + patterns = set()\n> + for instance in cls.subclasses:\n> + if hasattr(instance, 'patterns'):\n> + patterns.update(instance.patterns)\n> +\n> + return patterns\n> +\n> +\n> # ------------------------------------------------------------------------------\n> # Commit Checkers\n> #\n> \n> -class CommitChecker(metaclass=ClassRegistry):\n> - subclasses = []\n> -\n> - def __init__(self):\n> - pass\n> -\n> - #\n> - # Class methods\n> - #\n> - @classmethod\n> - def checkers(cls, commit, names):\n> - for checker in cls.subclasses:\n> - if names and checker.__name__ not in names:\n> - continue\n> - if checker.supports(commit):\n> - yield checker\n> -\n> - @classmethod\n> - def supports(cls, commit):\n> - return type(commit) in cls.commit_types\n> +class CommitChecker(CheckerBase):\n> + pass\n> \n> \n> class CommitIssue(object):\n> @@ -561,37 +581,8 @@ class TrailersChecker(CommitChecker):\n> # Style Checkers\n> #\n> \n> -class StyleChecker(metaclass=ClassRegistry):\n> - subclasses = []\n> -\n> - def __init__(self):\n> - pass\n> -\n> - #\n> - # Class methods\n> - #\n> - @classmethod\n> - def checkers(cls, filename, names):\n> - for checker in cls.subclasses:\n> - if names and checker.__name__ not in names:\n> - continue\n> - if checker.supports(filename):\n> - yield checker\n> -\n> - @classmethod\n> - def supports(cls, filename):\n> - for pattern in cls.patterns:\n> - if fnmatch.fnmatch(os.path.basename(filename), pattern):\n> - return True\n> - return False\n> -\n> - @classmethod\n> - def all_patterns(cls):\n> - patterns = set()\n> - for checker in cls.subclasses:\n> - patterns.update(checker.patterns)\n> -\n> - return patterns\n> +class StyleChecker(CheckerBase):\n> + pass\n> \n> \n> class StyleIssue(object):\n> @@ -748,37 +739,8 @@ class ShellChecker(StyleChecker):\n> # Formatters\n> #\n> \n> -class Formatter(metaclass=ClassRegistry):\n> - subclasses = []\n> -\n> - def __init__(self):\n> - pass\n> -\n> - #\n> - # Class methods\n> - #\n> - @classmethod\n> - def formatters(cls, filename, names):\n> - for formatter in cls.subclasses:\n> - if names and formatter.__name__ not in names:\n> - continue\n> - if formatter.supports(filename):\n> - yield formatter\n> -\n> - @classmethod\n> - def supports(cls, filename):\n> - for pattern in cls.patterns:\n> - if fnmatch.fnmatch(os.path.basename(filename), pattern):\n> - return True\n> - return False\n> -\n> - @classmethod\n> - def all_patterns(cls):\n> - patterns = set()\n> - for formatter in cls.subclasses:\n> - patterns.update(formatter.patterns)\n> -\n> - return patterns\n> +class Formatter(CheckerBase):\n> + pass\n> \n> \n> class CLangFormatter(Formatter):\n> @@ -957,7 +919,7 @@ def check_file(top_level, commit, filename, checkers):\n> after = commit.get_file(filename)\n> \n> formatted = after\n> - for formatter in Formatter.formatters(filename, checkers):\n> + for formatter in Formatter.instances(filename, checkers):\n> formatted = formatter.format(filename, formatted)\n> \n> after = after.splitlines(True)\n> @@ -971,7 +933,7 @@ def check_file(top_level, commit, filename, checkers):\n> \n> # Check for code issues not related to formatting.\n> issues = []\n> - for checker in StyleChecker.checkers(filename, checkers):\n> + for checker in StyleChecker.instances(filename, checkers):\n> checker = checker(after)\n> for hunk in commit_diff:\n> issues += checker.check(hunk.side('to').touched)\n> @@ -1019,7 +981,7 @@ def check_style(top_level, commit, checkers):\n> issues = 0\n> \n> # Apply the commit checkers first.\n> - for checker in CommitChecker.checkers(commit, checkers):\n> + for checker in CommitChecker.instances(commit, checkers):\n> for issue in checker.check(commit, top_level):\n> print('%s%s%s' % (Colours.fg(Colours.Yellow), issue.msg, Colours.reset()))\n> issues += 1\n> -- \n> Regards,\n> \n> Laurent Pinchart\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 0D23BC3304\n\tfor ;\n\tMon, 21 Oct 2024 12:44:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7CE1565392;\n\tMon, 21 Oct 2024 14:43:58 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C156E6538A\n\tfor ;\n\tMon, 21 Oct 2024 14:43:55 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C4BF3316;\n\tMon, 21 Oct 2024 14:42:09 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"nfRF1L1b\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1729514529;\n\tbh=MLyvsRRWPuSJtxNmr5CmNt9cbSkw+J3qdp/CMv/KnVE=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=nfRF1L1bi0xkqy5WIe75Ji39Ntervxx1oTbGfaeEa09AejwTc0tNnly3dMoKEtQID\n\tAmOiPSNG7qudSWjq5ReSSXnaPeSBLZjmkVCWUoig6QXvM0oR4UN4DfCKYrRGnJOPyl\n\tTZWDn57GoQNLovRDU1jV/RkGSYr0/LEm+35QwuRc=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20241018193246.805-2-laurent.pinchart@ideasonboard.com>","References":"<20241018193246.805-1-laurent.pinchart@ideasonboard.com>\n\t<20241018193246.805-2-laurent.pinchart@ideasonboard.com>","Subject":"Re: [PATCH 1/4] utils: checkstyle.py: Factor out common code to new\n\tCheckerBase class","From":"Kieran Bingham ","Cc":"Stefan Klug ","To":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Mon, 21 Oct 2024 13:43:52 +0100","Message-ID":"<172951463225.3674438.282375606387170899@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]