[libcamera-devel] [PATCH v2 3/6] checkstyle: Introduce a Commit class
Nicolas Dufresne
nicolas.dufresne at collabora.com
Fri Jan 17 23:32:54 CET 2020
Le samedi 18 janvier 2020 à 00:12 +0200, Laurent Pinchart a écrit :
> Hi Nicolas,
>
> Thank you for the patch.
>
> On Fri, Jan 17, 2020 at 02:17:30PM -0500, Nicolas Dufresne wrote:
> > From: Nicolas Dufresne <nicolas.dufresne at collabora.com>
> >
> > This introduce a Commit class used in the final revlist list. All the
> > git command are moved into that class. This class will be used to
> > introduce new type of commit (index and amendment) needed to implement
> > pre-commit hook support.
> >
> > Signed-off-by: Nicolas Dufresne <nicolas.dufresne at collabora.com>
> > ---
> > utils/checkstyle.py | 43 +++++++++++++++++++++++++++++--------------
> > 1 file changed, 29 insertions(+), 14 deletions(-)
> >
> > diff --git a/utils/checkstyle.py b/utils/checkstyle.py
> > index e7375b3..fb865c8 100644
> > --- a/utils/checkstyle.py
> > +++ b/utils/checkstyle.py
> > @@ -458,12 +458,34 @@ class StripTrailingSpaceFormatter(Formatter):
> > # Style checking
> > #
> >
> > +class Commit:
> > + commit = None
>
> This isn't needed. This field would be accessed through Commit.commit,
> which isn't equivalent to self.commit below.
Oops, you are right. I'm rusty in python.
>
> > +
> > + def __init__(self, commit):
> > + self.commit = commit
> > +
> > + def get_info(self, top_level):
> > + # Get the commit title and list of files.
> > + ret = subprocess.run(['git', 'show', '--pretty=oneline', '--name-
> > only',
> > + self.commit],
> > + stdout=subprocess.PIPE).stdout.decode('utf-8')
> > + files = ret.splitlines()
> > + return files[0], files[1:]
>
> How about
>
> ret = ret.splitlines()
> title = ret[0]
> files = ret[1:]
> return title, files
>
> to show what we're returning ?
I'd use a comment for that instead (it's interpreted language after all), what
about:
# returning title and files list as a tuple
>
> > +
> > + def get_diff(self, top_level, filename):
> > + return subprocess.run(['git', 'diff', '%s~..%s' % (self.commit,
> > self.commit),
> > + '--', '%s/%s' % (top_level, filename)],
> > + stdout=subprocess.PIPE).stdout.decode('utf-
> > 8')
> > +
> > + def get_file(self, filename):
> > + return subprocess.run(['git', 'show', '%s:%s' % (self.commit,
> > filename)],
> > + stdout=subprocess.PIPE).stdout.decode('utf-
> > 8')
> > +
> > +
> > def check_file(top_level, commit, filename):
> > # Extract the line numbers touched by the commit.
> > - diff = subprocess.run(['git', 'diff', '%s~..%s' % (commit, commit), '
> > --',
> > - '%s/%s' % (top_level, filename)],
> > - stdout=subprocess.PIPE).stdout
> > - diff = diff.decode('utf-8').splitlines(True)
> > + diff = commit.get_diff(top_level, filename)
> > + diff = diff.splitlines(True)
> > commit_diff = parse_diff(diff)
> >
> > lines = []
> > @@ -476,9 +498,7 @@ def check_file(top_level, commit, filename):
> >
> > # Format the file after the commit with all formatters and compute the
> > diff
> > # between the unformatted and formatted contents.
> > - after = subprocess.run(['git', 'show', '%s:%s' % (commit, filename)],
> > - stdout=subprocess.PIPE).stdout
> > - after = after.decode('utf-8')
> > + after = commit.get_file(filename)
> >
> > formatted = after
> > for formatter in Formatter.formatters(filename):
> > @@ -522,12 +542,7 @@ def check_file(top_level, commit, filename):
> >
> >
> > def check_style(top_level, commit):
> > - # Get the commit title and list of files.
> > - ret = subprocess.run(['git', 'show', '--pretty=oneline','--name-only',
> > commit],
> > - stdout=subprocess.PIPE)
> > - files = ret.stdout.decode('utf-8').splitlines()
> > - title = files[0]
> > - files = files[1:]
> > + title, files = commit.get_info(top_level)
> >
> > separator = '-' * len(title)
> > print(separator)
> > @@ -576,7 +591,7 @@ def extract_revlist(revs):
> > revlist = ret.stdout.decode('utf-8').splitlines()
> > revlist.reverse()
> >
> > - return revlist
> > + return [Commit(x) for x in revlist]
>
> As you're returning commits, should this function be renamed to
> extract_commits ?
>
> With these issues addressed,
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> >
> >
> > def git_top_level():
More information about the libcamera-devel
mailing list