-
Notifications
You must be signed in to change notification settings - Fork 43
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
Better Settings derive macro #666
Better Settings derive macro #666
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Idea to improve the messages for SettingsError
, if the context doesn't already come from elsewhere
use thiserror::Error; | ||
|
||
#[derive(Error, Debug)] | ||
pub enum SettingsError { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not new code, but taking advantage of my outsider mindset, these improvements come to mind:
Will there be enough context in the error messages? I know that with Anyhow it may well be supplied by chained errors, but in case we're not using it, as a user, I imagine reading these messages after I pass a big nested chunk of json...
#[serde(rename_all = "camelCase")] | ||
pub struct UserSettings { | ||
#[serde(rename = "user")] | ||
#[settings(nested)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the generated docs I cannot find what (nested)
means, I am looking at "Derive Macro agama_settings::Settings
"
I see only
#[derive(Settings)]
{
// Attributes available to this derive:
#[settings]
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have checked how serde documents the Serialize
and Deserialize
macros and they have the same problem. I will try to have a closer look, but it is something we can postpone a bit.
Co-authored-by: Martin Vidner <martin@vidner.net>
Co-authored-by: Martin Vidner <martin@vidner.net>
* They contain more information about the error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I try to test this with agama config
CLI, it fails (mostly by failing to report my mistakes, more on that later) but it also fails the same way in master
, so probably not fault of this PR.
I still don't understand the metaprogramming.
On the plus side:
- the generated code compiles
- tests pass, there are new tests
- better error messages
- better rustdoc
OK, then it is something we need to improve. Do you have an invocation example? Thanks!
You will understand it much better when you have to write your first derive macro 😅 |
For integration tests of the settings code, I should have used # echo '{"user":{"fullName":"tux","userName":"Tux","password":"secret","autologin":true}}' > user.json
# agama config load user.json
I, [2023-08-03T12:11:02.347584 #16187] INFO -- manager: Setting first user tux
# agama -f yaml config show
user:
fullName: tux
userName: Tux
password: secret
autologin: true
... Well... it also hides the errors :-/ # echo '{"user":{"shortname":"tux","userName":["Tux"],"password":"secret","autologin":42}}' > userbad.json
# agama config load userbad.json |
Problems
Settings
derive macro generates some code, it cannot generate code forInstallSettings
orNetworkSettings
.crate::settings::Settings
, which does not work from another package; changing it toagama_lib::settings::Settings
would make it fail for agama-lib.Solution
Making the macro reusable
To make the derive macro reusable and easy to test, I moved it to a separate package,
agama-settings
. Now we can implement an integration test.In the future, we could refactor the code to make it easier to test the internals (checking the generated code, error handling, and so on). I got some ideas to improve the implementation from Structuring, testing and debugging procedural macro crates.
Extending the macro
This PR extends the macro to handle "scalar" (e.g., a string, a boolean, a number), "nested" (e.g., InstallSettings#network) and "collection" attributes (e.g., NetworkSettings#connections).
By now, I have decided to rely on some attributes instead of inferring which kind of settings are (scalar, nested or collections). The reason is that you do not have access to the type system, just to the syntax tree. You can assume that
Vec
is a collection, but what if you are parsingstd::vec::Vec
instead? OK, I know it is not the best example, but I guess you get what I meant.If we decide to try to infer the values, it is as easy as changing the parse_setting_fields function with something else.
Improving the error reporting
The
SettingsError
struct has been changed to make it easier to report errors like the example below:Testing
If you are curious enough, you can generate the code produced by the macro:
cargo expand -p agama-lib > expanded.rs
Hints for reviewers
Look for the deleted code implementing
impl Settings for FooSettings
because that shows you concrete examples of what the macro should expand to.