On Tue, 2 Jun 2020 at 15:25, Martin Liška <mli...@suse.cz> wrote:
>
> On 6/2/20 4:14 PM, Jonathan Wakely wrote:
> > On Tue, 2 Jun 2020 at 14:56, Jonathan Wakely <jwakely....@gmail.com> wrote:
> >>
> >> On Tue, 2 Jun 2020 at 14:16, Martin Liška <mli...@suse.cz> wrote:
> >>>
> >>> On 6/2/20 1:48 PM, Martin Liška wrote:
> >>>> I tend to this approach. Let me prepare a patch candidate for it.
> >>>
> >>> There's a patch for it. Can you please Jonathan take a look?
> >>
> >> Looks great, thanks!
> >>
> >> + if name not in wildcard_prefixes:
> >> + msg = 'unsupported wilcard prefix'
> >>
> >> Typo "wilcard"
> >>
> >> + if pattern not in used_patterns:
> >> + self.errors.append(Error('a file pattern not used in a
> >> patch',
> >> + pattern))
> >>
> >> If the script printed this error I don't think I'd know what it was
> >> complaining about. How about "pattern doesn't match any changed files"
> >> ?
> >>
> >> I find the existing error 'file not changed in a patch' to be
> >> suboptimal too. We're checking revisions (or commits), not patches.
> >
> > Along those lines, here's an incomplete patch (tests aren't updated
> > yet, no commit log, your latest change isn't merged ) to revise the
> > error messages. I've tried to make them more consistent (e.g change
> > 'file not changed in a patch' to 'unchanged file mentioned in a
> > ChangeLog' which mirrors the error for the opposite condition,
> > 'changed file not mentioned in a ChangeLog').
>
> I welcome this.
>
> >
> > I've also moved line numbers to the start of the line (like GCC's own
> > diagnostics) and not used the "line" argument of the Error constructor
> > for things that aren't line numbers. I've aimed to be consistent, so
> > that line numbers come at the start, pathnames and patterns come at
> > the end (and there's a space before them).
>
> Well, the Error ctor argument 'line' is bit misleading. It's a string and
> not an integer:
>
> File "/home/marxin/Programming/gcc/contrib/gcc-changelog/git_commit.py",
> line 177, in __repr__
> return '%d: %s' % (self.line, self.message)
> TypeError: %d format: a number is required, not str
>
> and it represents a line from a git commit message. I use it in order to
> provide
> line which is affected by an error.
>
> >
> > I also suggest changing 'additional author must prepend with tab and 4
> > spaces' to 'additional author must be indented with one tab and four
> > spaces'.>
> > The intent is to improve the user experience, since for many people
> > who are new to git *any* error can cause confusion, so extra,
> > gcc-specific errors that we issue should aim to be easy to understand.
>
> I like your wordings.
OK, here's a proper patch for the changes you liked, dropping the
changes to the Error exception type.
pytest contrib/gcc-changelog/test_email.py passes.
OK for master?
commit 49652b7f5b57b88c1e0e27cf8ac488cbc85f1c7d
Author: Jonathan Wakely <jwak...@redhat.com>
Date: Tue Jun 9 21:25:50 2020 +0100
gcc-changelog: Improve git_commit.py diagnostics
This changes some error messages to be more self-consistent and to fix
some grammar.
contrib/ChangeLog:
* gcc-changelog/git_commit.py (GitCommit.parse_changelog):
Improve error strings.
* gcc-changelog/test_email.py: Update expected errors.
diff --git a/contrib/gcc-changelog/git_commit.py
b/contrib/gcc-changelog/git_commit.py
index f85d4c83c63..0b350ba7fda 100755
--- a/contrib/gcc-changelog/git_commit.py
+++ b/contrib/gcc-changelog/git_commit.py
@@ -377,8 +377,8 @@ class GitCommit:
elif additional_author_regex.match(line):
m = additional_author_regex.match(line)
if len(m.group('spaces')) != 4:
- msg = 'additional author must prepend with tab ' \
- 'and 4 spaces'
+ msg = 'additional author must be indented with '\
+ 'one tab and four spaces'
self.errors.append(Error(msg, line))
else:
author_tuple = (m.group('name'), None)
@@ -438,15 +438,14 @@ class GitCommit:
m = star_prefix_regex.match(line)
if m:
if len(m.group('spaces')) != 1:
- err = Error('one space should follow asterisk',
- line)
- self.errors.append(err)
+ msg = 'one space should follow asterisk'
+ self.errors.append(Error(msg, line))
else:
last_entry.lines.append(line)
else:
if last_entry.is_empty:
msg = 'first line should start with a tab, ' \
- 'asterisk and space'
+ 'an asterisk and a space'
self.errors.append(Error(msg, line))
else:
last_entry.lines.append(line)
@@ -527,7 +526,7 @@ class GitCommit:
used_patterns = set()
for entry in self.changelog_entries:
if not entry.files:
- msg = 'ChangeLog must contain at least one file entry'
+ msg = 'no files mentioned for ChangeLog in directory'
self.errors.append(Error(msg, entry.folder))
assert not entry.folder.endswith('/')
for file in entry.files:
@@ -540,7 +539,8 @@ class GitCommit:
if not self.is_changelog_filename(x[0])]
changed_files = set(cand)
for file in sorted(mentioned_files - changed_files):
- self.errors.append(Error('file not changed in a patch', file))
+ msg = 'unchanged file mentioned in a ChangeLog'
+ self.errors.append(Error(msg, file))
for file in sorted(changed_files - mentioned_files):
if not self.in_ignored_location(file):
if file in self.new_files:
diff --git a/contrib/gcc-changelog/test_email.py
b/contrib/gcc-changelog/test_email.py
index 04ddad3f100..df57bb5c94a 100755
--- a/contrib/gcc-changelog/test_email.py
+++ b/contrib/gcc-changelog/test_email.py
@@ -105,7 +105,7 @@ class TestGccChangelog(unittest.TestCase):
email = self.from_patch_glob('0096')
assert email.errors
err = email.errors[0]
- assert err.message == 'file not changed in a patch'
+ assert err.message == 'unchanged file mentioned in a ChangeLog'
assert err.line == 'gcc/testsuite/gcc.target/aarch64/' \
'advsimd-intrinsics/vdot-compile-3-1.c'
@@ -161,8 +161,8 @@ class TestGccChangelog(unittest.TestCase):
def test_additional_author_list(self):
email = self.from_patch_glob('0342')
- assert (email.errors[1].message == 'additional author must prepend '
- 'with tab and 4 spaces')
+ assert (email.errors[1].message == 'additional author must be indented
'
+ 'with one tab and four spaces')
def test_trailing_whitespaces(self):
email = self.get_git_email('trailing-whitespaces.patch')
@@ -260,8 +260,8 @@ class TestGccChangelog(unittest.TestCase):
def test_wrong_changelog_entry(self):
email = self.from_patch_glob('0020-IPA-Avoid')
- assert (email.errors[0].message
- == 'first line should start with a tab, asterisk and space')
+ msg = 'first line should start with a tab, an asterisk and a space'
+ assert (email.errors[0].message == msg)
def test_cherry_pick_format(self):
email = self.from_patch_glob('0001-c-Alias.patch')
