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

feat(core::props::Props): add function "get_ref" #89

Merged
merged 1 commit into from
Feb 12, 2025

Conversation

hasezoey
Copy link
Contributor

ISSUE NUMBER - PULL_REQUEST_TITLE

No Issue

Description

This PR adds a get_ref function to Props to get the searched attribute by reference instead of always cloning like get does.
Also add tests for all set and get function of Props.

I did not include a get_ref_or function as it is simple enough to just chain props.get_ref(query).unwrap_or(default).

Personally, i would recommend to maybe remove the original get function in a major version and replace it with get_ref, this way it is up to the user when to clone the returned value.
If it should be removed, i would recommend to put a #[deprecated] on get before it being removed.

Type of change

Please select relevant options.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Checklist

  • My code follows the contribution guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings
  • I formatted the code with cargo fmt
  • I checked my code using cargo clippy and reports no warnings
  • I have added tests that prove my fix is effective or that my feature works
  • I have introduced no new C-bindings
  • The changes I've made are Windows, MacOS, UNIX, Linux compatible (or I've handled them using cfg target_os)
  • I increased or maintained the code coverage for the project, compared to the previous commit

This change would for example allow another clone at stdlib#TextSpan to be removed.
And likely other places too.

to get the AttrValue by reference instead of always cloning.
Also add tests for all set / get functions.
@hasezoey
Copy link
Contributor Author

I also just noticed that MockComponent::query also returns a full AttrValue instead of a reference, maybe that could be changed too?

@veeso veeso merged commit 9b78962 into veeso:main Feb 12, 2025
3 of 4 checks passed
@hasezoey hasezoey deleted the propsByRef branch February 12, 2025 09:11
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