Create a Public Inbox Plugin - #329
Conversation
That's nice
I think you can create a test fetching your own emails on a single day on the kernel mailing list; shouldn't overload the mail archive server and should be pretty quick if you choose a day with a single email and no long threads on it.
Makes sense to me using the markdown format for adding links. |
Yeah, thanks for working on this improvement!
So far it seems no
Agreed: Choosing a narrow since/until interval on a public public-inbox instance should be completely sufficient I'd say.
Sounds good to me to provide the rich output including links in the |
479af44 to
3e328bf
Compare
|
I've just updated the branch taking your comments into account:
|
There was a problem hiding this comment.
Thanks much for the changes! Looks very good. I'm proposing just a couple of rather minor adjustments in 339d788. Could you please have a look if the changes are ok? Included is the --verbose mode for showing the full url of the message. I would like to keep this formatting (e.g. one-line per item by default) consistent across all plugins.
|
|
||
| return msgs | ||
|
|
||
| def __fetch_thread_root(self, msg: Message) -> Message: |
Check notice
Code scanning / CodeQL
Explicit returns mixed with implicit (fall through) returns
It looks great to me, thanks! |
|
@mripard thanks for this PR. Really useful! I used this plugin for a while without issues, but I just hit one today while running My configuration is: For now I have This is the error: As rapid fix, I applied the following patch, but I'm not sure if it is the right thing to do: diff --git a/did/plugins/public_inbox.py b/did/plugins/public_inbox.py
index ec8e333..d3df993 100644
--- a/did/plugins/public_inbox.py
+++ b/did/plugins/public_inbox.py
@@ -106,7 +106,10 @@ class PublicInbox(object):
item(self._get_message_url(msg), level=2, options=opt)
def __get_mbox_from_content(self, content: bytes) -> mailbox.mbox:
- content = gzip.decompress(content)
+ try:
+ content = gzip.decompress(content)
+ except gzip.BadGzipFile:
+ return {}
with tempfile.NamedTemporaryFile() as tmp:
tmp.write(content) |
|
@stefano-garzarella Hi, I'm glad it's helpful. I've pushed a new version that should fix it, plus a unit test to cover the failing case (we were ignoring the HTTP request status code and assuming the result was a GZIP archive). I've also squashed the patch @psss did earlier. |
|
/packit test |
|
Hi, is there anything I can help with to get this merged? |
|
Sorry for late response, just too many things on my plate... I checked the latest version but I get the following error when running the test: def __get_thread_root(self, msg: Message) -> Message:
log.debug("Looking for thread root of message %s" % msg.id())
if msg.is_thread_root():
log.debug("Message is thread root already. Returning.")
return msg
parent_id = msg.parent_id()
if parent_id not in self.messages_cache:
root = self.__fetch_thread_root(msg)
log.debug("Found root message %s for message %s" % (root.id(), msg.id()))
return root
while True:
log.debug("Parent is %s" % parent_id)
> assert parent_id in self.messages_cache
E AssertionError
../../did/plugins/public-inbox.py:158: AssertionErrorCould you please have a look at it? Also, I'm not sure it's a good idea to call the plugin file |
I think I fixed it already but wasn't sure whether I should push it or wait for it to be merged and then send it. I guess I have my answer :)
I don't have an opinion here, but also I'm not sure how to do that in Python. Would you have any pointers? |
:)
It would be just slightly modifying the way how plugins are imported. The relevant code should be around the Line 115 in 070a535 |
|
I just pushed a new version with |
|
Is there anything else I can do to help get this merged? |
psss
left a comment
There was a problem hiding this comment.
Thanks for the fix and sorry for the late response. Looks good. Added just a few last nitpicks.
|
Thanks for the suggestions, I've added your suggestions or reworked the code to address your concerns. |
Some plugins like public-inbox might use dashes in their name to be more user-friendly, but that will break python module loading system. Let's replace all the dashes in the plugin names by underscores. Signed-off-by: Maxime Ripard <mripard@kernel.org>
|
Thanks for addressing the comments! Now looks good. I've just modified the first commit summary and removed the prefix, see the recommendations for some more context. |
Public Inbox is a mailing-list archive project notably used by the Linux Foundation to host the lore.kernel.org ML archive. This plugins allows to fetch from those archives the threads that the user started or was involved in and integrate them into the report. Signed-off-by: Maxime Ripard <mripard@kernel.org>
|
@mripard I just received this error today: Any idea what is wrong? |
|
Sorry for the late answer. I don't have an immediate answer. Could you share your config (or at least the lore part) so I can reproduce it? |
Sure, my config contains: I had the issue running For now, I'm using the following workaround: diff --git a/did/plugins/public_inbox.py b/did/plugins/public_inbox.py
index 888f41c..70e1233 100644
--- a/did/plugins/public_inbox.py
+++ b/did/plugins/public_inbox.py
@@ -150,6 +150,9 @@ class PublicInbox(object):
parent_id = msg.parent_id()
if parent_id not in self.messages_cache:
root = self.__fetch_thread_root(msg)
+ if root is None:
+ return None
+
log.debug("Found root message %s for message %s" % (root.id(), msg.id()))
return root
@@ -164,6 +167,9 @@ class PublicInbox(object):
parent_id = parent.parent_id()
if parent_id not in self.messages_cache:
root = self.__fetch_thread_root(msg)
+ if root is None:
+ return None
+
log.debug(
"Found root message %s for message %s" %
(root.id(), msg.id()))
@@ -205,6 +211,9 @@ class PublicInbox(object):
if not msg.is_thread_root():
root = self.__get_thread_root(msg)
+ if root is None:
+ continue
+
root_id = root.id()
if root_id in found:
log.debug("Root message already encountered... Skip.") |
|
@stefano-garzarella @mripard it seems that issue fixed at mripard#1 happens again. I see that the latest code that landed does not check if @stefano-garzarella IMO your code is not a workaround but the proper fix since the problem is AFAIU when the thread is broke by some MUA or the list and there's no root message in the thread to reference anymore. |
@martinezjavier I'm not sure if |
|
I've spent some more time looking into it. The offending mail you first reported this issue for is: https://lore.kernel.org/all/AS2P194MB2170A39251AC72302E8F6F459ACB2@AS2P194MB2170.EURP194.PROD.OUTLOOK.COM/ and it looks like the culprit is due to root message not being archived by lore, and thus while the message has a correct in-reply-to chain, lore doesn't give us the root message. I've started to work on a proper fix (that includes fixing the type hints too, since a significant part were wrong / poorly handled), and indeed, I think that yielding the current message directly when we don't have a root makes it a bit nicer than just ignoring it. |
yeah, that would be much better! Thanks for working on a proper fix! |
Public Inbox is a mailing-list archive project notably used by the Linux Foundation to host the lore.kernel.org ML archive.
This plugins allows to fetch from those archives the threads that the user started or was involved in and integrate them into the report.
This is a draft PR because there's a couple of things I'm unsure of and would like some feedback on:
Thanks!