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 option for skipping fields #76

Merged
merged 4 commits into from
Aug 3, 2021
Merged

Conversation

pnevyk
Copy link
Contributor

@pnevyk pnevyk commented Jul 23, 2021

Implements #72

The idea is similar to serde's skip. The getter/setter is not implemented for fields that are marked #[getset(skip)]. So for:

#[derive(CopyGetters)]
#[get_copy]
pub struct Plain {
    #[getset(skip)]
    non_copyable: String,
    copyable: usize,
}

only fn copyable(&self) -> usize getter is generated. This feature makes sense for larger structs, where using struct-level attribute is convenient for brevity, but the user still wants to ignore a subset of fields. It is possible to achieve this behavior in the current version by annotating each field with a corresponding getset attribute and not annotating the fields for which the getter/setter should not be generated.

This PR lacks thorough testing and documentation, I wanted to be sure this is a way to go before writing the tests and docs.

Open questions I can think of:

  1. Current implementation accepts multiple identifiers inside getset attribute. For the example above, it means that if I use #[getset(get_copy, skip)], it silently swallows the "get_copy" part and skips the field, but if I use #[getset(skip, get_copy)], the last identifier (i.e., "get_copy") is taken and the getter is generated (resulting into a compilation error for moving a non-copyable value). This is inconsistent and confusing for the user. I think it should be forbidden to use any other identifier if skip is present, but there is also point 2.
  2. A user might want to skip only a particular getter/setter. For example, the struct could derive both Getters and Setters and the user wants to omit the setter part on a field. Should getset support this too by a more granular syntax (e.g., getset(skip(get)) or get(skip) or getset(skip_get)) or is it out of scope? (The getset(skip_get) syntax is inspired by serde's skip_serializing.)

@pnevyk
Copy link
Contributor Author

pnevyk commented Jul 25, 2021

The code now checks whether some getter or setter is used with skip and gives a compilation error if that is the case. I believe this is the least confusing behavior as it disallows the user to use a getter/setter with skip, possibly thinking that it somehow influences the generation. This made the implementation of parse_attr a bit more complicated and less nice. Maybe there is a better way to do it?

I leave the "skip a specific getter/setter" feature out of the PR.

Any ideas how this can be tested further?

Copy link
Collaborator

@Hoverbear Hoverbear left a comment

Choose a reason for hiding this comment

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

This looks good, thanks @pnevyk :)

@Hoverbear Hoverbear merged commit f081d39 into jbaublitz:master Aug 3, 2021
@pnevyk pnevyk deleted the skip-attribute branch August 3, 2021 19:15
@pnevyk
Copy link
Contributor Author

pnevyk commented Aug 3, 2021

Great, thank you too

@Hoverbear Hoverbear added this to the 0.1.2 milestone Nov 28, 2021
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