Provides two macros to make model creation more convenient #643
Conversation
gabsi26
changed the title
|
Thank you for the pull request, @gabsi26! I'll take a look later, possibly tomorrow. |
There was a problem hiding this comment.
Thank you again for the pull request, @gabsi26! This is an awesome improvement!
I left a bunch of comments with specific suggestions, as well as more general remarks on where I'd like to see things going. Feel free to address those in a follow-up PR, if you're up for it. I'll merge this one now, as it provides an excellent base for further iteration.
| "crates/fj-viewer", | ||
| "crates/fj-proc", | ||
|
|
There was a problem hiding this comment.
The rest of that list is alphabetically sorted. Would be nice to keep that. Also, no need for an empty line.
| "crates/fj-proc", | ||
|
|
||
|
|
There was a problem hiding this comment.
There's the same crate a second time. I'm surprised that doesn't result in some kind of error! 😄
|
|
||
| [dependencies.fj-proc] | ||
| path = "../../crates/fj-proc" |
There was a problem hiding this comment.
I think it should be possible, to re-export fj-proc from fj (pub use fj_proc;). Then we wouldn't need the extra dependency, and would also have a simpler syntax in the models (fj::model instead of fj_proc::model).
| bracketed, parenthesized, parse::Parse, parse_macro_input, parse_quote, | ||
| }; | ||
|
|
||
| pub fn attributed_arguments(_: TokenStream, input: TokenStream) -> TokenStream { |
There was a problem hiding this comment.
I think having two different attributes (and two different models for testing/demonstrating) them is overkill. I like the approach implemented in this one much better. It would be better, if this just replaced the other one.
There was a problem hiding this comment.
The first one was more or less just left in because it was the first one I came up with. So more for completeness than anything else. I'll get rid of it :)
| _ => {} | ||
| } | ||
| } else { | ||
| default = Some(value.val.clone()); |
There was a problem hiding this comment.
Maybe it would be better not to have this fallback, and always require an explicit default = to specify default values. Would be more verbose in the simple case, but also easier to read and understand.
What do you think?
There was a problem hiding this comment.
I think I'll simplify the macro to not allow for this implicit fallback
| default = Some(value.val.clone()); | ||
| } | ||
| } | ||
| let [default, min, max] = determine_default(default, min, max); |
There was a problem hiding this comment.
I think the behavior of this function, using min or max as the default, if no default is provided, is unexpected and potentially confusing for users. It would probably be clearer to just have no default, if none is provided, and fail with an error message, if the user doesn't provide a value for a default-less parameter.
| /// Represents one parameter given to the `model` | ||
| /// `#[value(default=3, min=4)] num_points: u64` | ||
| /// `^^^^^^^^^^^^^^^^^^^^^^^^^^ ~~~~~~~~~~ ^^^-- ty` | ||
| /// ` | |` | ||
| /// ` attr ident` |
There was a problem hiding this comment.
Nice style of documentation 👍
With this change the `model` function is a lot nicer to write default values are provided as macro arguments currently all arguments **must** have a default value defined this way there are also no checks in place to ensure the default is valid for the type of the argument Signed-off-by: gabsi26 <gabriel.seebach@gmx.at>
Signed-off-by: gabsi26 <gabriel.seebach@gmx.at>
Signed-off-by: gabsi26 <gabriel.seebach@gmx.at>
Signed-off-by: gabsi26 <gabriel.seebach@gmx.at>
This is a first iteration on a asuggested way to implement modelling convenience Signed-off-by: gabsi26 <gabriel.seebach@gmx.at>
Signed-off-by: gabsi26 <gabriel.seebach@gmx.at>
The struct `AttributeParameter`was only used for its `new` method, thus it was replaced by `determine_default()` Some comments were added to make the purpose of structs a bit clearer Signed-off-by: gabsi26 <gabriel.seebach@gmx.at>
Signed-off-by: gabsi26 <gabriel.seebach@gmx.at>
hannobraun
force-pushed
the
proc_me_up_scotty
branch
from
20a4dd9
to
78b4756
Compare
|
One additional note: I just discovered, that No big deal, I'll fix it (I'm about to do my weekly |
An initial PR to get this rolling.
This PR deals with #72. Where
attributed_argumentsis a crude implementation of an idea #72 (comment)The attribute
#[value()]is required for each parameter the user wants to pass to the model it accepts different forms#[value(3)]sets default value to 3#[value(default=3)]same as abovemin and max can additionally be supplied like this:
#[value(min=1, max=2)]As long as either one of the three values is supplied a default will be set, min and max are currently only used if no default is provided to set it. Where min takes precedence over max.
Later on a doc comment containing the provided min and max values should be introduced.