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 setting doc & attribute for each field separately #6

Draft
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

cyqsimon
Copy link
Contributor

@cyqsimon cyqsimon commented Jun 8, 2023

Summary

Basically, this PR would allow you to do this:

#[optfield(Opt)]
struct Foo {
    #[optfield_filed(attrs = (bar), doc = "69420")]
    foo: String,
}

... which generates

struct Opt {
    #[bar]
    #[doc = "69420"]
    foo: Option<String>,
}

Checklist

  • implementation
  • documentation
  • relevant tests

@cyqsimon
Copy link
Contributor Author

cyqsimon commented Jun 8, 2023

I've also added documentation append mode for good measure. So now you can do this:

/// 69
#[optfield(Opt, doc = append("420"))]
struct Foo {
    /// nice
    #[optfield_field(doc = append("nicer"))]
    foo: String,
}

this generates

#[doc = " 69"]
#[doc = "420"]
struct Opt {
    #[doc = " nice"]
    #[doc = "nicer"]
    foo: Option<String>,
}

@roignpar
Copy link
Owner

roignpar commented Jun 8, 2023

Thank you for the PR. I encourage you in the future to first open a feature request issue before putting in work for a PR.
This feature opens up a discussion similar to the one found here: #2

Although tested, it is not documented (maybe it should) that optfield can be applied multiple times to the same struct.
Adding individual field level attributes/features has the potential to significantly increase the complexity of the API and thus I am reluctant to opening up this path.

However, I will take a closer look and give this some thought in the near future.

I hope this doesn't come across as a lack of appreciation for your effort. I recommend that any further work on this PR be postponed until I come to a conclusion regarding whether we should pursue this further. There is a real possibility that this will not be merged, so I think it's best to avoid doing more work before it is decided to add this to optfield.

@cyqsimon
Copy link
Contributor Author

cyqsimon commented Jun 8, 2023

Thanks for a quick reply, and please do take your time to think about whether this feature is a good inclusion or not.


I completely understand your concerns on future maintainability. Personally I hate janky code in general for the same reasons, hence why I'm here coding in Rust. While I cannot say that I have foreseen every possible pitfall, I can assure you that I've tried my best to keep things extensible, and that I did not make any unidiomatic/haphazard design decisions.

So for example, the allowed arguments on the fields have a distinct container type, so that we can trivially restrict what field arguments are allowed (at the cost of having some code duplication, but that's easily solvable with a trait if necessary).

https://github.com/roignpar/optfield/pull/6/files#diff-c87a21ba2f9ece98fdf2a45ccb30dbbd2c20983a5dfb8355c216d90dc9892046R10-R14

Also here I completely refactored this trait because the logic in generate was getting wayyy out of hand:

https://github.com/roignpar/optfield/pull/6/files#diff-40ed46a58372b854f42a41668e1e203c2195b3d833013172512125ec25540e4cL10-L84

I opted to make the trait much simpler and put most of the attribute generation logic into each impl, which I think turned out quite nicely. A big match block is much more maintainable than cascading if blocks IMO.

https://github.com/roignpar/optfield/pull/6/files#diff-4743d8ef499500da7aa3ce7a6b2edd463780365d1cd9b1db1f14a4f5197d5fc1R59-R127

In terms of the specific logic, everything I have now is just a simple per-field override. So for example if both the struct
and a particular field have attrs = add(...) args, the one on the struct simply has no effect on this field and there's nothing complex/weird going on.

Hopefully this can help reassure you that I haven't sacrificed too much maintainability to implement this feature. And I do intend to add documentation and full test coverage too.


And finally, thanks for giving me a heads-up on the possibility that this will not be merged. That's not a problem with me and I fully respect your decision. However I'll still go ahead and finish the tests and documentation on my branch, just to tie up loose ends.

Please don't misinterpret this as me trying to pressure you; it's just that I personally need this feature for my project, and I'd rather it has proper tests and documentation than not. So yeah, since I'm doing this for myself anyway, might as well offer to upstream the feature. Again, I fully respect your choice in this matter; if necessary, I'm okay with having a git dependency that pulls directly from my branch indefinitely.

@cyqsimon
Copy link
Contributor Author

cyqsimon commented Jun 21, 2023

An update (and note to future self):

It seems like some extra work is required for now, because proc_macro_attribute does not yet support helper attributes like proc_macro_derive does. See rust-lang/rust#65823. So what I've written now compiles but the feature isn't actually usable. Bummer.

There is an available workaround that involves "consuming" the field attributes (i.e. use it then remove it), but as you mentioned, it gets a little tricky when optfield attribute is used on the same struct multiple times. That being said I think I can make it work though, and I actually think it has some benefits (namely, allowing multiple optfield invocations on the same struct to have different field attributes on the same field). I'll try to come up with a prototype.

@cyqsimon
Copy link
Contributor Author

cyqsimon commented Jun 25, 2023

The workaround has been implemented, with surprisingly few changes I must say. As is the case previously, this is intended to work fine with multiple optfield attributes on the same struct, but this capability is not documented.

I still have tests to add, but you can already try it out right now.

I've had to use a different attribute name for the field attribute (optfield_field, yuck) though due to namespace issues. I asked on Rust's community Discord guild, and it seems like without explicit support for helper attributes from proc_macro_attribute, this is just an ugly fact we have to deal with. Oh well.

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