Extract macro to new crate #93
Conversation
|
@idanarye I am not sure about what to do with the bottom types for repeated and required fields: #31 (comment) You said that you would like to remove the one for required fields and keep the one for repeated fields, but on the other hand, the error for repeated fields points to the method with the name of the field. Therefore, we could just replace both bottom types with |
| } | ||
| pub use typed_builder_macro::TypedBuilder; | ||
|
|
||
| #[doc(hidden)] |
There was a problem hiding this comment.
Now that it's an official trait in its own crate, I think it should be exposed and documented. I can add the documentation myself later though.
There was a problem hiding this comment.
To be honest, I don't see a value in exposing the trait. Adding it to the documentation whould be a distraction since it is only used internally in the code generated by the macro.
But adding a normal comment would be nice.
There was a problem hiding this comment.
This is mostly a direct copy-paste, right? (github won't show a proper diff because of the new src/lib.rs...)
There was a problem hiding this comment.
Yes, I would mention changes that are not obvious from the commit messages.
There was a problem hiding this comment.
I did replace make_punctuated_single with the implementation of FromIterator. That is not obvious.
|
This is weird, but now that I check again the repeated value error does not show the type in the rust-analyzer virtual text diagnostics and missing value does:
This is the opposite of what I've observed in #31. But it's actually better, because repeated value is easy to figure out even without the type name as the hint. This means that I no longer mind having a |
|
Because of this change which might be dependent on the editor, I personally would prefer leaving things as are and migrating to a better, less hacky option as you mentioned later. Therefore, I mark this PR as ready to merge, but if you want to remove the repeated field enums, I can still do it. |
|
@idanarye Thanks for merging! We might need to adjust the CI if we add tests to the macro crate. I think we should use a workspace and add |
|
Why? All the tests are still main crate, not the proc-macro crate. They still run with a regular |
|
I just thought about the scenario of adding a test to subcrates that would not be run in the CI. Aslong as we keep all tests there, there is no problem. |
|
All the tests go in the |
Closes #31 (comment)
Updated the edition to 2021 and removed
extern crate proc_macro;because it did not help at the end: #57 (comment)