News Hook failing to detect empty trivial news fragments

[ichard26]

The problem

PyPA-bot incorrectly fails the news-file/pr status on any PR that is trivial and uses an empty (whatever).trivial.rst named file in the news directory to denote that. The root cause is that unidiff doesn't parse empty file additions in the diff correctly (see matiasb/python-unidiff#19).

For example, PR pypa/pip#9425 's diff looks like this:

diff --git a/news/1ab8f1c8-c115-4055-9a60-30a8f8eef7ba.trivial.rst b/news/1ab8f1c8-c115-4055-9a60-30a8f8eef7ba.trivial.rst
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/src/pip/_internal/utils/logging.py b/src/pip/_internal/utils/logging.py
index 687bfe3b2f..df5a8eead8 100644
--- a/src/pip/_internal/utils/logging.py
+++ b/src/pip/_internal/utils/logging.py
@@ -187,7 +187,7 @@ def should_color(self):
         return False
 
     def format(self, record):
-        msg = logging.StreamHandler.format(self, record)
+        msg = super().format(record)
 
         if self.should_color():
             for level, color in self.COLORS:
@@ -215,7 +215,7 @@ class BetterRotatingFileHandler(logging.handlers.RotatingFileHandler):
 
     def _open(self):
         ensure_dir(os.path.dirname(self.baseFilename))
-        return logging.handlers.RotatingFileHandler._open(self)
+        return super()._open()
 
 
 class MaxLevelFilter(Filter):
diff --git a/tests/lib/path.py b/tests/lib/path.py
index a9dc29ad7a..259293d0e3 100644
--- a/tests/lib/path.py
+++ b/tests/lib/path.py
@@ -22,8 +22,8 @@ class Path(str):
 
     def __new__(cls, *paths):
         if len(paths):
-            return str.__new__(cls, os.path.join(*paths))
-        return str.__new__(cls)
+            return super().__new__(cls, os.path.join(*paths))
+        return super().__new__(cls)
 
     def __div__(self, path):
         """
@@ -74,7 +74,7 @@ def __repr__(self):
         return "Path({inner})".format(inner=str.__repr__(self))
 
     def __hash__(self):
-        return str.__hash__(self)
+        return super().__hash__()
 
     @property
     def name(self):

The valid trivial news fragment is only denoted in the diff with the following header:

diff --git a/news/1ab8f1c8-c115-4055-9a60-30a8f8eef7ba.trivial.rst b/news/1ab8f1c8-c115-4055-9a60-30a8f8eef7ba.trivial.rst
new file mode 100644
index 0000000000..e69de29bb2

The thing is that unidiff looks for the --- a/some_path and +++ b/some_path lines to know whether a new file diff has started so the trivial news fragment is missed:

>>> import unidiff
>>> with open("pr9425.diff", "r", encoding="utf8") as f:
...     diff = unidiff.PatchSet(f.read())
... 
>>> len(diff)
2
>>> for f in diff:
...     print(repr(f))
... 
<PatchedFile: src/pip/_internal/utils/logging.py>
<PatchedFile: tests/lib/path.py>

(technically in this case the trivial news fragment diff merged with the file diff after it, but in other cases the trivial news fragment diff is completely lost)

(A few) Solutions

1. The hook can fetch the list of files a PR touched using the GitHub API and then search the list for a matching file addition or modification.
2. Using some hacky string manipulation and regex matching, the lost header-only diffs can be recovered from the raw diff. The recovered diff headers then can be checked if they denote a valid news fragment was added or modified.

The only reason I bring up the second option is just in case there's some limits that the first option would hit like API request limits or webhook response time requirements.

/cc @pradyunsg I heard that you noticed this bug and wasn't able to identify the root cause; I hope this helps!

[pradyunsg]

Nice! Thanks a lot for looking into this @ichard26! ^.^