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

Updated derive to support all serde skip_* attributes #83

Merged
merged 3 commits into from
Oct 9, 2023

Conversation

wspeirs
Copy link
Contributor

@wspeirs wspeirs commented Aug 24, 2023

I was running into an issue where I wanted a field to be skipped for both inserts and selects, but #[serde(skip)] wasn't working. This change simply checks the attribute to see if it starts with skip. A bit of a hack as it does not support skip_serialization_if, but meets my needs, and moves the bar.

@wspeirs
Copy link
Contributor Author

wspeirs commented Oct 4, 2023

@loyd just wondering if there's any feedback on this... thanks!

@@ -3,7 +3,7 @@ use quote::quote;
use serde_derive_internals::{attr::get_serde_meta_items, Ctxt};
use syn::{parse_macro_input, Data, DataStruct, DeriveInput, Fields, Ident, Lit, Meta, NestedMeta};

/// Parses `#[serde(skip_serializing)]`
/// Parses `#[serde(skip_*)]`; does not work properly with `#[serde(skip_serializing_if)]`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's explicitly allow skip_deserializing to avoid a possible footgun for users.

@loyd
Copy link
Collaborator

loyd commented Oct 5, 2023

@wspeirs, oh, sorry for an inadequately long answer, and ty for your contribution. LGTM, but let's disable skip_serializing_if explicitly.

@wspeirs
Copy link
Contributor Author

wspeirs commented Oct 6, 2023

OK, made it explicitly skip_serializing and skip_deserializing, which is I think what you were asking. Feel free to change/modify as you see fit... thanks!

@loyd loyd merged commit 58d7093 into ClickHouse:master Oct 9, 2023
loyd added a commit that referenced this pull request Oct 9, 2023
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