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

Support #[builder(getter(...))] attribute #222

Merged
merged 18 commits into from
Dec 1, 2024

Conversation

lazkindness
Copy link
Contributor

@lazkindness lazkindness commented Nov 28, 2024

Resolves #221

Implements an experimental (and limited) version of a getter attribute to return references to already set values.

Currently supported (for structs, methods, and functions):

  • #[builder(getter)]
  • #[builder(getter(name = "new_getter_name"))]
  • #[builder(getter(vis = "pub(crate)"))]
  • #[builder(getter(doc {}))]
  • ...or any combination of the above 3

In its current form it's not very "smart", so getters for a String return &String and not &str, for example, nor is copy / clone inferred and implemented for types which support them.

See #221 for some more discussion on the future features.

@lazkindness lazkindness marked this pull request as ready for review November 29, 2024 01:33
Comment on lines 49 to 50
#[allow(unused)]
Inferred,
Copy link
Collaborator

@Veetaha Veetaha Nov 29, 2024

Choose a reason for hiding this comment

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

This should probably clarify what Inferred means and why it's unused in a comment. Although see #222 (comment)

Comment on lines 3 to 7
pub(crate) const DOCS_CONTEXT: &str = "builder struct's impl block";

pub(crate) fn parse_docs(meta: &syn::Meta) -> Result<super::SpannedKey<Vec<syn::Attribute>>> {
crate::parsing::parse_docs_without_self_mentions(DOCS_CONTEXT, meta)
}
Copy link
Collaborator

@Veetaha Veetaha Nov 29, 2024

Choose a reason for hiding this comment

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

I don't think it makes sense to extract this module. It's okay to just copy this small function into the getter module. The DOCS_CONTEXT isn't meant to be a cross-module-shared variable with such a generic name, it's meant to be module-local and describe the context where the docs will be pasted for the config param defined in that module

Comment on lines 55 to 57
fn from_none() -> Option<Self> {
None
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't this already the default implementation of this trait method in FromMeta?

bon-macros/src/builder/builder_gen/member/config/getter.rs Outdated Show resolved Hide resolved
bon-macros/src/builder/builder_gen/member/config/getter.rs Outdated Show resolved Hide resolved
website/src/reference/builder/member/getter.md Outdated Show resolved Hide resolved
website/src/reference/builder/member/getter.md Outdated Show resolved Hide resolved
website/src/reference/builder/member/getter.md Outdated Show resolved Hide resolved
website/src/reference/builder/member/getter.md Outdated Show resolved Hide resolved
@lazkindness
Copy link
Contributor Author

Thanks for the feedback. I'm about done aside from that one comment above, just going to sleep now and finish up the tests in the morning.

@lazkindness lazkindness requested a review from Veetaha November 29, 2024 21:00
Copy link
Collaborator

@Veetaha Veetaha left a comment

Choose a reason for hiding this comment

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

LGTM, although I did just a quick review this time. Once new comments are fixed I'll merge this. I'll re-review the changes tomorrow, improve any other minor stuff, maybe update some of the guides, and do the minor release.

website/src/reference/builder/member/getter.md Outdated Show resolved Hide resolved
website/.vitepress/config.mts Outdated Show resolved Hide resolved
bon-macros/src/builder/builder_gen/getter.rs Outdated Show resolved Hide resolved
bon-macros/src/builder/builder_gen/getter.rs Outdated Show resolved Hide resolved
@lazkindness lazkindness requested a review from Veetaha November 29, 2024 21:35
@lazkindness
Copy link
Contributor Author

LGTM, although I did just a quick review this time. Once new comments are fixed I'll merge this. I'll re-review the changes tomorrow, improve any other minor stuff, maybe update some of the guides, and do the minor release.

Cool, I do also have a need for the copy and clone attributes so some time this weekend or next week I can probably take a stab at those if you don't get to it already

@lazkindness
Copy link
Contributor Author

Pipeline should be good now too, I added a tiny bit to the init script and contributing docs

@Veetaha
Copy link
Collaborator

Veetaha commented Dec 1, 2024

I'm pushing my amendments and merging

"`getter` attribute is experimental and requires \
\"experimental-getter\" cargo feature to be enabled; \
we would be glad to make this attribute stable if you find it useful; \
please leave a 👍 reaction under the issue https://github.com/elastio/bon/issues/221 \
Copy link

@beyera beyera Dec 1, 2024

Choose a reason for hiding this comment

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

I believe you want to reference the new stabilize tracking issue here instead.

Suggested change
please leave a 👍 reaction under the issue https://github.com/elastio/bon/issues/221 \
please leave a 👍 reaction under the issue https://github.com/elastio/bon/issues/225 \

Copy link
Collaborator

@Veetaha Veetaha Dec 1, 2024

Choose a reason for hiding this comment

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

Yeah, I updated this code locally, although I can't push my changes to Kindness-Works:laz/221-getters even though there is this sentence in the PR metadata:
image

$ git push Kindness-Works pr/lazkindness/222:laz/221-getters
ERROR: Permission to Kindness-Works/bon.git denied to Veetaha.
fatal: Could not read from remote repository.

Please make sure you have the correct access rights

Maybe you have an org-wise rejection for writes from external contributors?

Anyway, I'll force-merge this PR and submit my amendments in a separate PR

Opened a followup PR #226

cc @lazkindness

# See more info at https://bon-rs.com/reference/builder/member/getter.
#
# We are considering stabilizing this attribute if you have a use for it. Please leave
# a 👍 reaction under the issue https://github.com/elastio/bon/issues/221 if you need
Copy link

Choose a reason for hiding this comment

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

Same here?

Suggested change
# a 👍 reaction under the issue https://github.com/elastio/bon/issues/221 if you need
# a 👍 reaction under the issue https://github.com/elastio/bon/issues/225 if you need

@Veetaha Veetaha merged commit 4ea25ab into elastio:master Dec 1, 2024
21 of 27 checks passed
@Veetaha
Copy link
Collaborator

Veetaha commented Dec 1, 2024

Opened a followup PR #226

@Veetaha
Copy link
Collaborator

Veetaha commented Dec 2, 2024

This feature has been released in bon 3.2.0. Thank you for the contribution!

@Veetaha
Copy link
Collaborator

Veetaha commented Dec 5, 2024

@lazkindness I opened an (unfinished yet) draft PR for clone, copy, deref params support: #229. Notifying you to make sure we don't duplicate the effort

@Veetaha
Copy link
Collaborator

Veetaha commented Dec 7, 2024

Heads up, did a 3.3.0 release with copy, clone, deref support: https://github.com/elastio/bon/releases/tag/v3.3.0.

cc @lazkindness

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.

Support getters for already set members
3 participants