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

relax JSON string arg types #85

Merged
merged 4 commits into from
Dec 18, 2023
Merged

Conversation

robfitzgerald
Copy link
Collaborator

@robfitzgerald robfitzgerald commented Dec 18, 2023

edit

this PR modified the signature of methods in config_json_extension.rs so that string arguments are references and they can be String or str types. this makes it so cloning of key names is no longer required for using these methods and at the same time, allows 'static str arguments, which greatly reduces boilerplate for some applications.

original post

Nick - started this work to relax what we can pass as arguments to the JSON getter extensions we have. this

  1. removes the need to .clone() keys a lot
  2. relax the types we can pass as arguments, so that &str and &String arguments are both valid:
let parent_1 = String::from("iteration termination function");
let x = config.get_config_i64(&"limit", &parent_1);

just letting you know about it, it's going to touch a lot of files, just making sure this doesn't mess with anything you are up to this morning.

Closes #79.

@robfitzgerald robfitzgerald marked this pull request as ready for review December 18, 2023 18:14
@robfitzgerald
Copy link
Collaborator Author

@nreinicke mkay this one is ready.

i was able to get the arguments to be flexible by making them &dyn AsRef<str>, which both String and str can be. the implementation of AsRef for String becomes

impl AsRef<str> for String {
    #[inline]
    fn as_ref(&self) -> &str {
        self
    }
}

so i think this should be a zero-cost abstraction for both types.

Copy link
Collaborator

@nreinicke nreinicke left a comment

Choose a reason for hiding this comment

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

Nice, this really cleans things up! I especially like that we can do &"config_key" rather than String::from("config_key"), much simpler!

@robfitzgerald robfitzgerald merged commit b05e55a into main Dec 18, 2023
5 checks passed
@robfitzgerald robfitzgerald deleted the rjf/reduce-cloning-in-configuration branch December 18, 2023 18:34
@robfitzgerald
Copy link
Collaborator Author

totes! also, another thing i slipped in was an implementation of AsRef<str> for CompassConfigurationField, so that those can also be passed directly. if there are other enums out there that become JSON key lookup values, we can create AsRef instances for those to get that same behavior.

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.

over-use of cloning on things that could be dereferenced or simply passed by reference
2 participants