Skip to content
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

Add new custom_values field #147

Merged
merged 3 commits into from
Feb 9, 2025
Merged

Add new custom_values field #147

merged 3 commits into from
Feb 9, 2025

Conversation

yg0x01
Copy link
Contributor

@yg0x01 yg0x01 commented Feb 9, 2025

According to the documentation, add compatible custom values content from the latest version.

@LukaOber
Copy link
Collaborator

LukaOber commented Feb 9, 2025

Is there a reason I am missing for using

#[serde(skip_serializing_if = "Option::is_none")]
custom_values: Option<Vec<CompositeValue>>,

instead of

#[serde(skip_serializing_if = "Vec::is_empty")]
custom_values: Vec<CompositeValue>,

@yg0x01
Copy link
Contributor Author

yg0x01 commented Feb 9, 2025

Sorry, I didn't think it through carefully and just followed the examples around. Using skip_serializing_if = "Vec::is_empty" for the Vec<_> type instead of Option<Vec<_>> is simpler. I have corrected it.

@LukaOber
Copy link
Collaborator

LukaOber commented Feb 9, 2025

Is the push syntax the better choice here? You implemented this.

pub fn custom_values<C: Into<CompositeValue>>(mut self, custom_value: C) -> Self {
    self.custom_values.push(custom_value.into());
    self
}

But do we actually want to push several times, or would the following be the better approach? I think the latter would be better for setting custom values. What do you think? How are you going to use it?

pub fn custom_values<C: Into<CompositeValue>>(mut self, custom_value: C) -> Self {
    self.custom_values = custom_values.into_iter().map(|c| c.into()).collect();
    self
}

@yg0x01
Copy link
Contributor Author

yg0x01 commented Feb 9, 2025

I wish I could directly set this array without needing to loop and use push. However, after looking at how vectors are handled in other places, I noticed that many use push. I think it would be better to provide both methods for vectors: one for push and another for directly setting the array. However, this could be a lot of work. For this case, I prefer the direct setting approach. I have modified the code based on your suggestion.

@LukaOber
Copy link
Collaborator

LukaOber commented Feb 9, 2025

Sorry for all the hassle, thank you for opening and implementing my suggestions.

@LukaOber LukaOber merged commit b7a1a14 into yuankunzhang:main Feb 9, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants