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

Fix problems with global attributes and implements getter/setter for Option<T> #83

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

hochej
Copy link

@hochej hochej commented Feb 6, 2022

I took a closer at the problems that occur if global and local attributes are used that result in the same function name (as mentioned in #68 and #74).
In the following I will briefly explain my proposal.

  1. We reduce the interface to only one derive macro (#[derive(GetSet)]). This allows to fix Confusing error when #[getset(get_copy = "pub")] is specified but not derive(CopyGetters) #67 automatically and avoids breaking changes
  2. Attributes/Parameters are stored in a HashMap (e.g. HashMap<GetSetAttr, Meta>) and mutually exclusive parameters have to be equal and share the same hash. This allows to parse all global parameter to begin and then for each field the local parameters are added to a clone of the global HashMap.
    Two parameters are compared against each other based on their identifier() that is used in fn eq() and fn hash().
pub struct GetSetAttr {
    mode: GetSetMode,
    prefix: Option<&'static str>,
}

impl GetSetAttr {
    pub fn identifier(&self) -> &'static str {
        if let Some(prefix) = self.prefix {
            return prefix;
        }
        match self.mode {
            GetSetMode::Get | GetSetMode::GetCopy | GetSetMode::GetOption => "get",
            GetSetMode::GetMut => "get_mut",
            GetSetMode::Set | GetSetMode::SetOption => "set",
            GetSetMode::Skip => "skip",
        }
    }
}

Otherwise, one could also directly use the later function name for comparison, this should be equivalent.
3. I have implemented the proposal of #78 with get_option and also set_option. This would make my other PR obsolete.

I haven't touched any of the existing functions in order to not introduce any breaking changes. This unfortunately leads to some boilerplate code at the moment, but I hope this is acceptable.

I would really appreciate to hear your opinion about this PR. If you agree with the ideas, I'd be happy to write some new tests and I could also add some new examples to the Readme.

commit adds the following modifications:
- fixes bug (jbaublitz#67) and implements #[derive(GetSet)]
- allows overwriting of global getters with get_copy/get_option
- implements get_option/set_option as proposed in issue 78
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.

Confusing error when #[getset(get_copy = "pub")] is specified but not derive(CopyGetters)
1 participant