docs: improve docstrings #6496

WofWca · 2025-02-01T16:52:21Z

The first commit is a follow-up to #6072.

For the second commit I did not read through the docstrings thorougly, I simply copied them from payload.rs and moved around slightly.

Follow-up to #6072.

Replicate the docstrings from `src/events/payload.rs`.

Hocuri

Yay for better documentation!

I have some comments, but I approved already because it's nothing big, so if you think you adequately addressed them you are free to directly merge. Otherwise, I'll look at it again.

In general, it's good to request somebody's review on core PRs so that they don't get forgotten; if in doubt, @link2xt and me is always good, and also GitHub usually suggests some reviewers.

Hocuri · 2025-02-04T18:47:20Z

deltachat-jsonrpc/src/api/types/events.rs

+    IncomingMsg {
+        /// ID of the chat where the message is assigned.
+        chat_id: u32,
+
+        /// ID of the message.
+        msg_id: u32,
+    },


Not sure about these two new comments, at least when I am reading the code they don't add any new information. Like, when i see a field chat_id, then it's clear to me that this is the ID of the chat where something is happening.

I thought so as well, but I kept it because they're in payload.rs, I wanted them to be in sync.

No let's keep it as-is to keep things in sync, I'll do a follow-up PR.

Hocuri · 2025-02-04T18:48:06Z

deltachat-jsonrpc/src/api/types/events.rs

+        /// ID of the chat which the message belongs to.
+        chat_id: u32,
+
+        /// ID of the message that was successfully sent.
+        msg_id: u32,


Same here, at least to me these comments don't add any new information. Same for the comments below.

Hocuri · 2025-02-04T18:51:37Z

deltachat-jsonrpc/src/api/types/events.rs

-    MsgDeleted { chat_id: u32, msg_id: u32 },
+    MsgDeleted {
+        /// ID of the chat where the message was prior to deletion.
+        /// Never 0 or trash chat.


The trash chat is just an internal implementation detail of core, to the UIs a trashed message doesn't exist, so I don't think it's not necessary to mention it here.

Also, mentioning here that the chat_id is never 0 sounds like the other similar events (MsgRead etc.) sometimes are sometimes emitted with chat_id=0 or msg_id=0?

are sometimes emitted with chat_id=0 or msg_id=0?

Judging from DC Desktop code, this can be the case for at least ReactionsChanged and MsgsChanged. See the code.

Yes, at least MsgsChanged does sometimes have chat_id=0 also in practice. But the events around a message state change (MsgRead, MsgFailed, MsgDelivered, MsgsNoticed) do not IIRC.

But anyways, as you said, you just copied it, I'll make a follow-up PR that fixes these things in both locations.

Hocuri · 2025-02-04T18:52:57Z

deltachat-jsonrpc/src/api/types/events.rs

    #[serde(rename_all = "camelCase")]
-    LocationChanged { contact_id: Option<u32> },
+    LocationChanged {
+        /// contact_id of the contact for which the location has changed.


Suggested change

/// contact_id of the contact for which the location has changed.

/// The contact for which the location has changed.

It's clear now that it's a contact_id, I think this can be removed

I agree, it can be removed. Feel free to apply suggestion, though keep in mind about the point of "keeping files in sync".

deltachat-jsonrpc/src/api/types/events.rs

Hocuri · 2025-02-04T18:56:02Z

deltachat-jsonrpc/src/api/types/events.rs

+        /// Message ID.
        msg_id: u32,
+
+        /// Status update ID.
        status_update_serial: u32,


Also here, a comment Message ID on a field msg_id and Status update ID on status_update_serial doesn't add any new information, at least when I'm reading it.

Hocuri · 2025-02-04T18:56:47Z

deltachat-jsonrpc/src/api/types/events.rs

+        /// Message ID.
+        msg_id: u32,
+
+        /// Realtime data.
+        data: Vec<u8>,


Hocuri · 2025-02-04T18:57:49Z

deltachat-jsonrpc/src/api/types/events.rs


    /// Inform that a message containing a webxdc instance has been deleted
    #[serde(rename_all = "camelCase")]
-    WebxdcInstanceDeleted { msg_id: u32 },
+    WebxdcInstanceDeleted {
+        /// ID of the deleted message.


Suggested change

/// ID of the deleted message.

/// Message ID of the deleted webxdc instance.

in order to keep consistent wording

Hocuri · 2025-02-04T19:04:54Z

deltachat-jsonrpc/src/api/types/events.rs

+    ChatlistItemChanged {
+        /// ID of the changed chat
+        chat_id: Option<u32>,


Also here, not necessary to say comment ID of the changed chat when the event is called ChatlistItemChanged and the field chat_id

WofWca

As I said in the replies, I, for the most part, simply copied the docs from payload.rs, making the docs synced.

If you think it's fine to make some adjustments, feel free to commit / implement the suggestions, I have no objections in this regard.

WofWca · 2025-02-04T19:06:41Z

deltachat-jsonrpc/src/api/types/events.rs

+    IncomingMsg {
+        /// ID of the chat where the message is assigned.
+        chat_id: u32,
+
+        /// ID of the message.
+        msg_id: u32,
+    },


I thought so as well, but I kept it because they're in payload.rs, I wanted them to be in sync.

deltachat-jsonrpc/src/api/types/events.rs

WofWca · 2025-02-04T19:11:49Z

deltachat-jsonrpc/src/api/types/events.rs

-    MsgDeleted { chat_id: u32, msg_id: u32 },
+    MsgDeleted {
+        /// ID of the chat where the message was prior to deletion.
+        /// Never 0 or trash chat.


are sometimes emitted with chat_id=0 or msg_id=0?

Judging from DC Desktop code, this can be the case for at least ReactionsChanged and MsgsChanged. See the code.

WofWca · 2025-02-04T19:13:43Z

deltachat-jsonrpc/src/api/types/events.rs

    #[serde(rename_all = "camelCase")]
-    LocationChanged { contact_id: Option<u32> },
+    LocationChanged {
+        /// contact_id of the contact for which the location has changed.


I agree, it can be removed. Feel free to apply suggestion, though keep in mind about the point of "keeping files in sync".

WofWca · 2025-02-04T19:18:24Z

If you think it's fine to make some adjustments, feel free to commit

Or just let me know if I should go ahead, and I'll clean this up myself

WofWca · 2025-02-06T20:39:38Z

So, do we merge as is?

Co-authored-by: WofWca <[email protected]>

Hocuri · 2025-02-06T21:35:53Z

Yes 👍

WofWca added 2 commits February 1, 2025 20:50

docs: improve docs for IncomingReaction event

fcd7026

Follow-up to #6072.

docs: improve JSON-RPC event docs

c82fcfb

Replicate the docstrings from `src/events/payload.rs`.

WofWca force-pushed the wofwca/event-docs branch from 80f6383 to c82fcfb Compare February 1, 2025 17:43

WofWca marked this pull request as ready for review February 1, 2025 17:44

Hocuri requested review from link2xt and Hocuri February 4, 2025 18:40

Hocuri approved these changes Feb 4, 2025

View reviewed changes

WofWca commented Feb 4, 2025

View reviewed changes

Update deltachat-jsonrpc/src/api/types/events.rs

c092e9c

Hocuri approved these changes Feb 6, 2025

View reviewed changes

Update deltachat-jsonrpc/src/api/types/events.rs

17a7ef0

Co-authored-by: WofWca <[email protected]>

WofWca enabled auto-merge (squash) February 10, 2025 09:40

link2xt disabled auto-merge February 14, 2025 23:10

link2xt merged commit 5c3d1e7 into main Feb 14, 2025
37 checks passed

link2xt deleted the wofwca/event-docs branch February 14, 2025 23:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docs: improve docstrings #6496

docs: improve docstrings #6496

WofWca commented Feb 1, 2025 •

edited

Loading

Hocuri left a comment •

edited

Loading

Hocuri Feb 4, 2025

WofWca Feb 4, 2025

Hocuri Feb 6, 2025

Hocuri Feb 4, 2025

Hocuri Feb 4, 2025

WofWca Feb 4, 2025

Hocuri Feb 6, 2025

Hocuri Feb 4, 2025

WofWca Feb 4, 2025

Hocuri Feb 4, 2025

Hocuri Feb 4, 2025

Hocuri Feb 4, 2025

Hocuri Feb 4, 2025

WofWca left a comment

WofWca Feb 4, 2025

WofWca Feb 4, 2025

WofWca Feb 4, 2025

WofWca commented Feb 4, 2025

WofWca commented Feb 6, 2025

Hocuri commented Feb 6, 2025

	/// contact_id of the contact for which the location has changed.
	/// The contact for which the location has changed.

	/// ID of the deleted message.
	/// Message ID of the deleted webxdc instance.

docs: improve docstrings #6496

docs: improve docstrings #6496

Conversation

WofWca commented Feb 1, 2025 • edited Loading

Hocuri left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WofWca left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WofWca commented Feb 4, 2025

WofWca commented Feb 6, 2025

Hocuri commented Feb 6, 2025

WofWca commented Feb 1, 2025 •

edited

Loading

Hocuri left a comment •

edited

Loading