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
- 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.
- 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!