Skip to content

Conversation

@Olegt0rr
Copy link
Contributor

@Olegt0rr Olegt0rr commented Mar 18, 2023

Close #863

Solution

  • add serde_with_macros::skip_serializing_none to every Telegram struct by default to exclude empty fields
  • add skip_serializing_if = "std::ops::Not::not" to exclude False to "True or absent" fields
  • add skip_serializing_if = "Vec::is_empty" to exclude empty [ ]

@Olegt0rr Olegt0rr changed the title Message struct skip serializing for none fields Telegram struct serializing similar to original (skip empty/defaults) Mar 18, 2023
@WaffleLapkin WaffleLapkin added S-waiting-on-review Status: Awaiting review from the assignee C-core crate: teloxide-core labels Mar 18, 2023
@WaffleLapkin WaffleLapkin self-requested a review March 18, 2023 22:48
@Olegt0rr
Copy link
Contributor Author

Olegt0rr commented Mar 19, 2023

@WaffleLapkin PR still not completed.
If the declared solution suits, I will fix it for other structures – let me know :)

Copy link
Contributor

@WaffleLapkin WaffleLapkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The general approach looks good to me! Most/all of these annotations I just forgot to add 😅

While round-tripping json through our structures preserving the exact form is not our primary goal, it's still good to have.

Also look out for our custom de/serialize impls and more typed reinterpretations (an enum instead of bunch of options, etc) — they historically caused a few bugs and can easily influence round-tripability.

/// For messages with a caption, special entities like usernames, URLs,
/// bot commands, etc. that appear in the caption.
#[serde(default)]
#[serde(skip_serializing_if = "Vec::is_empty", default = "Vec::new")]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW you don't need = "Vec::new", since Vec implements Default. If you could replace existing mentions of default = "Vec::new " with default that would be nice.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, fixed

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is fixed... have you maybe forgot to push the commit?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@WaffleLapkin, did you mean:

#[serde(skip_serializing_if = "Vec::is_empty", default)]

?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Olegt0rr you still did not address this comment, there are still default = "Vec::new" in the message.rs

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hirrolot I think this still wasn't addressed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just removed redundant "Vec::new"

@Olegt0rr Olegt0rr marked this pull request as ready for review March 21, 2023 17:27
@Olegt0rr Olegt0rr requested a review from WaffleLapkin March 21, 2023 17:33
@WaffleLapkin
Copy link
Contributor

Ping, @Olegt0rr do you still intend to work on this?

@WaffleLapkin WaffleLapkin added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author and removed S-waiting-on-review Status: Awaiting review from the assignee labels May 1, 2023
@Olegt0rr Olegt0rr requested a review from WaffleLapkin May 5, 2023 08:59
@hirrolot hirrolot added S-waiting-on-review Status: Awaiting review from the assignee and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author labels Jun 13, 2023
@hirrolot
Copy link
Member

@WaffleLapkin, could you review this PR?

@hirrolot hirrolot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author and removed S-waiting-on-review Status: Awaiting review from the assignee labels Jun 13, 2023
Copy link
Contributor

@WaffleLapkin WaffleLapkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@WaffleLapkin WaffleLapkin enabled auto-merge July 29, 2023 09:33
@WaffleLapkin WaffleLapkin added this pull request to the merge queue Jul 29, 2023
Merged via the queue into teloxide:master with commit 556b14e Jul 29, 2023
@Olegt0rr Olegt0rr deleted the fix-telegram-serialization branch July 29, 2023 14:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C-core crate: teloxide-core S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Telegram objects serialization is not correct

3 participants