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). --Sean
