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

Better Settings derive macro #666

Merged
merged 20 commits into from
Aug 3, 2023
Merged

Better Settings derive macro #666

merged 20 commits into from
Aug 3, 2023

Conversation

imobachgs
Copy link
Contributor

@imobachgs imobachgs commented Jul 17, 2023

Problems

  • Although our Settings derive macro generates some code, it cannot generate code for InstallSettings or NetworkSettings.
  • Writing tests for the macro is hard because it cannot be reused from external crates. The generated code refers to crate::settings::Settings, which does not work from another package; changing it to agama_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 parsing std::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:

# agama config set storage.lvm=fasle
Could not update 'storage.lvm': Invalid value 'fasle', expected a boolean

Testing

If you are curious enough, you can generate the code produced by the macro:

  1. Install cargo expand
  2. Run 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.

@coveralls
Copy link

coveralls commented Jul 17, 2023

Coverage Status

coverage: 72.158% (+0.08%) from 72.079% when pulling 47e76df on imobachgs:better-agama-derive into fa00c19 on openSUSE:master.

@imobachgs imobachgs marked this pull request as draft July 18, 2023 15:41
Copy link
Contributor

@mvidner mvidner left a 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 {
Copy link
Contributor

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...

rust/agama-settings/src/error.rs Show resolved Hide resolved
rust/agama-settings/src/error.rs Outdated Show resolved Hide resolved
rust/agama-settings/src/error.rs Show resolved Hide resolved
#[serde(rename_all = "camelCase")]
pub struct UserSettings {
#[serde(rename = "user")]
#[settings(nested)]
Copy link
Contributor

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]
}

Copy link
Contributor Author

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.

rust/agama-derive/src/lib.rs Show resolved Hide resolved
rust/agama-derive/src/lib.rs Show resolved Hide resolved
rust/agama-derive/src/lib.rs Show resolved Hide resolved
rust/agama-derive/src/lib.rs Outdated Show resolved Hide resolved
rust/agama-derive/src/lib.rs Outdated Show resolved Hide resolved
rust/agama-derive/src/lib.rs Outdated Show resolved Hide resolved
@imobachgs imobachgs marked this pull request as ready for review August 2, 2023 07:41
Copy link
Contributor

@mvidner mvidner left a 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

@imobachgs
Copy link
Contributor Author

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.

OK, then it is something we need to improve. Do you have an invocation example? Thanks!

I still don't understand the metaprogramming.

You will understand it much better when you have to write your first derive macro 😅

@imobachgs imobachgs merged commit 9c7a6ad into agama-project:master Aug 3, 2023
7 checks passed
@imobachgs imobachgs deleted the better-agama-derive branch August 3, 2023 11:30
@mvidner
Copy link
Contributor

mvidner commented Aug 3, 2023

For integration tests of the settings code, I should have used agama config load, it avoids the problems of config set mentioned in #688

# 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

@imobachgs imobachgs mentioned this pull request Aug 4, 2023
@imobachgs imobachgs mentioned this pull request Sep 26, 2023
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.

3 participants