Better Settings derive macro#666
Better Settings derive macro#666imobachgs merged 20 commits intoagama-project:masterfrom imobachgs:better-agama-derive
Conversation
mvidner
left a comment
There was a problem hiding this comment.
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.
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.
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.
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.
mvidner
left a comment
There was a problem hiding this comment.
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
Settingsderive macro generates some code, it cannot generate code forInstallSettingsorNetworkSettings.crate::settings::Settings, which does not work from another package; changing it toagama_lib::settings::Settingswould 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
Vecis a collection, but what if you are parsingstd::vec::Vecinstead? 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
SettingsErrorstruct 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.rsHints for reviewers
Look for the deleted code implementing
impl Settings for FooSettingsbecause that shows you concrete examples of what the macro should expand to.