-
Notifications
You must be signed in to change notification settings - Fork 3
Add an 'extras' property to comments #807
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@crazytonyli I think this is a good start, but it looks like we have quite a few failing tests.
- We could consider marking the field with
#[WpContextualOption]
because there won't always be extra fields. I wonder if that'd also help with de-serializing speed. - We might want to introduce a new type to hold the extras, instead of directly using
JsonValue
. This might be changed to be auniffi::Object
with some helpers in the future and having a type in advance could make things easier for us. It'd also look a bit more consistent when we use it in other types. Note that you probably need to use#[serde(transparent)]
for this to work, but I don't know if#[serde(transparent)]
&#[serde(flatten)]
works together or not. - The field ordering might matter, as I believe it does for de-serializing
enum
s. That'd be a bit of an issue, because if we accidentally put this field above others, the ones below might always be set toNone
. If this is an issue, we could add a check inWpContext
proc macro to ensure that if there is anextra
field, it's at the end. Having a specific type would also help with this. - If we have a separate type, we could consider moving the
meta
field to it. I am not so sure about this though, so just dropping it as an idea for now.
It does not seem so: https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=b62029454b6f50395fbb3dfd17a8d4b4. BTW, I'll leave this PR open until we have an API endpoint that requires this change, like the WP.com /wp/v2/comments (see https://linear.app/a8c/issue/AINFRA-939). |
450e9d2
to
56f6562
Compare
@oguzkocer This PR is ready to be reviewed and merged. The |
@@ -528,6 +528,9 @@ pub struct SparseComment { | |||
pub comment_type: Option<CommentType>, | |||
#[WpContext(edit, embed, view)] | |||
pub author_avatar_urls: Option<HashMap<UserAvatarSize, WpResponseString>>, | |||
#[serde(flatten)] | |||
#[WpContext(edit, embed, view)] | |||
pub additional_fields: Option<Arc<AnyJson>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This field is included in the generated SparseCommentFields
, which it should not be. I guess we'll have to implement a logic to exclude certain fields. We can either check the #[serde(flatten)]
attribute (if that's possible), or add a new attribute like #[WpContextualExcludeFromFields]
. What do you think? @oguzkocer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was checking what needed to be done for this and figured I'd quickly implement it, so you don't have to do the same: #833
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
I have got this error using the new attribute, do you mind having a look?
$ cargo build
Compiling wp_api v0.1.0 (/Users/tonyli/Projects/wordpress-rs/wp_api)
error[E0599]: no variant or associated item named `AdditionalFields` found for enum `SparseCommentFieldWithEditContext` in the current scope
--> wp_api/src/comments.rs:495:57
|
495 | #[derive(Debug, Serialize, Deserialize, uniffi::Record, WpContextual)]
| ^^^^^^^^^^^^
| |
| variant or associated item not found in `SparseCommentFieldWithEditContext`
| variant or associated item `AdditionalFields` not found for this enum
|
= note: this error originates in the derive macro `WpContextual` (in Nightly builds, run with -Z macro-backtrace for more info)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in c0cbafd. (after checking with @crazytonyli that it'd be OK for me to push directly to his branch)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@crazytonyli This is mostly looking good. I've left some suggestions. Let me know what you think!
@@ -528,6 +528,9 @@ pub struct SparseComment { | |||
pub comment_type: Option<CommentType>, | |||
#[WpContext(edit, embed, view)] | |||
pub author_avatar_urls: Option<HashMap<UserAvatarSize, WpResponseString>>, | |||
#[serde(flatten)] | |||
#[WpContext(edit, embed, view)] | |||
pub additional_fields: Option<Arc<AnyJson>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was checking what needed to be done for this and figured I'd quickly implement it, so you don't have to do the same: #833
56f6562
to
d685335
Compare
Exclude fields marked with WpContextualExcludeFromFields from integration test helper generation to prevent referencing non-existent enum variants.
Hey @oguzkocer , I have addressed your comments. Do you mind having another look? Thanks. |
@oguzkocer I tried the "serde flatten" solution you mentioned in our 1:1, and it worked out great. One minor downside is that there will be additional parsing work required for all API endpoints. I tried to use
RawValue
to avoid that, but it does not seem to work withserde(flatten)
(see serde-rs/json#1252).WP.com endpoints may return extra information in the responses. We'll need to parse that information to support the app's functionalities. This PR introduces a mechanism to support that. We can add this block of code to the models if needed: