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

Variable interpolation in the configuration file #71 #167

Merged
merged 9 commits into from
Sep 20, 2023

Conversation

danma3x
Copy link
Contributor

@danma3x danma3x commented Jul 11, 2023

Addresses #71

At this stage, now it's possible to have variables be properly substituted.
There are severe limitations to the way it is done in this PR.
Serde always expects a correct TOML for obvious reasons, so all variable mentions have to be in quotes for the toml to be formally correct.

At the time of substitution, quotes are added back for string type.
Due to the most of the options being on the top level, variable block has to be mentioned either on the bottom or alongside other blocks.
Example usage:

In configuration:

max_width = "$input_width"
border_color = "$border_color"
[variables]
input_width = 96
some_color = "blue"
border_colors = "black"

Due to the amount of changes, might need further work on error handling and control flow of configuration loading as well as a mention in README.md

@coastalwhite
Copy link
Owner

Foremost. Thank you very much for the PR. It is much appreciated.

I think this is implementation is good, although there are a few things I would like to change.

  • I would like to have the TOML syntax remain valid even if people use variables.
  • Doing replacement in the config string seems like that could get quite hairy quickly. I cannot, however, think of a much better solution at this moment. I am going to take a few days to think if there is a better solution to this.
  • Since the regex replacement is so simple, I think we should just write a small iterator manually. This avoids having a dependency on regex. Not so significant, but still.

An example of that first point being. As far as I understand now, this is currently valid syntax.

[variables]
a = 5

# ...

max_width = $a

Although, this syntax would be nice to have, it is not really valid TOML syntax anymore. We would really want "$a" to get replaced with 5 instead of "5". Maybe, with some custom serde deserialization function? Not sure though.

@danma3x
Copy link
Contributor Author

danma3x commented Jul 12, 2023

On the first point. The TOML syntax has to be valid in the first place, because we need a serde pass to parse the variables in the same file in the first place. Your example will produce an error at the point of trying to deserialize VariablesConfig because it will encounter an invalid toml value. The current behaviour is to look at the type of toml::Value in the VariablesConfig, and decide to re-add quotes based on the variant.

Basically, variables always have to be mentioned in quotes, and defined in the[variables] section with the value of the expected type. That's basically the only way to produce a valid TOML and have variable substitution that I came up with.

On the second point, true, I did say I considered it a quirky way to do it in the issue itself.

On the third point. Yeah, in hindsight, I should have just done that, and will probably do so soon.

@danma3x
Copy link
Contributor Author

danma3x commented Jul 13, 2023

I have added a variable iterator and removed the dependency on regex

@coastalwhite
Copy link
Owner

I apologize for coming back a bit late, I will look at this a bit later when I am less busy in other areas.

This commit changes the method in which variables are supplied and
parsed, requiring a separate file.

For the configuration file, this is first parsed into a RoughConfig
where variables might be inserted into any of the fields. Then,
variables are interpreted into a PartialConfig.
@coastalwhite
Copy link
Owner

Okay, finally got time to take a look at this.

Foremost, thank you for this PR. I changed quite a bit, but I am pretty sure I would have not touched this in quite a long time had you not laid most of the groundwork.

How it works now:

There is an additional file variables.toml which can be specified with the --variables flag or is by default expected at /etc/lemurs/variables.toml. This file gets loaded first and after the configuration will get loaded in 3 stages now.

  1. RoughConfig. Some typing information but may still contain variables.
  2. PartialConfig. Subset of the whole configuration.
  3. Config. Merged with default options.

This way we don't have to manually change TOML files and can rely on the toml crate to do all parsing. We just change the fields.

Again, thank you very much. I hope you will also use this feature yourself.

@coastalwhite coastalwhite linked an issue Sep 20, 2023 that may be closed by this pull request
@coastalwhite coastalwhite merged commit 780a27e into coastalwhite:main Sep 20, 2023
3 checks passed
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.

Add configuration variables
2 participants