Hi Sean, On Mon, 4 May 2020 at 10:59, Sean Anderson <sean...@gmail.com> wrote: > > On 5/4/20 10:39 AM, Simon Glass wrote: > > Hi Sean, > > > > On Sun, 3 May 2020 at 15:55, Sean Anderson <sean...@gmail.com> wrote: > >> > >> Patman outputs a line for every edition of the series in every patch, > >> regardless of whether any changes were made. This can result in many > >> redundant lines in patch changelogs, especially when a patch did not exist > >> before a certain revision. For example, the existing behaviour could result > >> in a changelog of > >> > >> Changes in v7: None > >> Changes in v6: None > >> Changes in v5: > >> - Make a change > >> > >> Changes in v4: None > >> > >> Changes in v3: > >> - New > >> > >> Changes in v2: None > >> > >> With this patch applied and with --no-empty-changes, the same patch would > >> look like > >> > >> (no changes since v5) > >> > >> Changes in v5: > >> - Make a change > >> > >> Changes in v3: > >> - New > >> > >> This is entirely aesthetic, but I think it reduces clutter, especially for > >> patches added later on in a series. > >> > >> Signed-off-by: Sean Anderson <sean...@gmail.com> > >> --- > >> > >> Changes in v3: > >> - Document empty changelog suppression in README > >> - Fix KeyError when running tests > >> - Fix no changes message being output for revision 1 > >> - Fix no changes message sometimes being output before every > >> non-newest-revision change > >> - Make the newest_version logic more robust (and ugly) > >> - Update commit subject > >> > >> Changes in v2: > >> - Add a note when there are no changes in the current revision > >> - Make this the default behaviour, and remove the option > >> > >> tools/patman/README | 21 ++++++++++++++++++++ > >> tools/patman/series.py | 44 +++++++++++++++++++++++++++++++----------- > >> 2 files changed, 54 insertions(+), 11 deletions(-) > > > > Reviewed-by: Simon Glass <s...@chromium.org> > > > > Please see comment below though. > > > > [..] > > > >> diff --git a/tools/patman/series.py b/tools/patman/series.py > >> index 6d9d48b123..4359442174 100644 > >> --- a/tools/patman/series.py > >> +++ b/tools/patman/series.py > >> @@ -146,38 +146,60 @@ class Series(dict): > >> Changes in v4: > >> - Jog the dial back closer to the widget > >> > >> - Changes in v3: None > >> Changes in v2: > >> - Fix the widget > >> - Jog the dial > >> > >> - etc. > >> + If there are no new changes in a patch, a note will be added > >> + > >> + (no changes since v2) > >> + > >> + Changes in v2: > >> + - Fix the widget > >> + - Jog the dial > >> """ > >> + versions = sorted(self.changes, reverse=True) > >> + newest_version = 1 > >> + try: > >> + newest_version = max(newest_version, int(self.version)) > >> + except (ValueError, KeyError): > >> + pass > >> + try: > >> + newest_version = max(newest_version, versions[0]) > >> + except IndexError: > >> + pass > > > > Can we do this without exceptions so it is more deterministic? > > > > E.g. > > > > if 'version' in self: > > newest_version = max(newest_version, int(self.version)) > > if versions: > > newest_version = max(newest_version, versions[0]) > > Is it fine to not check for ValueError in this instance? I noticed that > the other area where it's used also doesn't check, however that is in > DoChecks (and also doesn't check if the key exists either).
If the tests pass then you probably don't need checks. Having said that the test coverage is not 100%, so do a bit of manual testing too. I'm fine with raising an error if something is wrong. I just don't like using exceptions to figure out what to do. Regards, Simon
