make configuration handling a lot better #141
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #141 +/- ##
==========================================
- Coverage 18.90% 18.04% -0.86%
==========================================
Files 20 20
Lines 984 1003 +19
==========================================
- Hits 186 181 -5
- Misses 798 822 +24 ☔ View full report in Codecov by Sentry. |
odilia/src/main.rs
Outdated
| @@ -80,6 +85,42 @@ async fn main() -> eyre::Result<()> { | |||
| //initialize a task tracker, which will allow us to wait for all tasks to finish | |||
| let tracker = TaskTracker::new(); | |||
|
|
|||
| tracing::debug!("Reading configuration"); | |||
There was a problem hiding this comment.
Could you move the creation of the application config to a new function in config? Seems more fitting. In fact, you could even implement this as ApplicationConfigBuilder::default() or something like that, as it constructs a config from various steps.
There was a problem hiding this comment.
I don't think that's a very good idea. As I see it, the configuration is supposed to be plane objects, models as in an mvc pattern for example, essentially just data.
Adding methods to that object would imply that the configuration knows how to build itself, in other words, has additional responsabilities. This is contrary to the principles I go by writing code, because essentially config is indeed just a typed data store, and nothing more than that. That's why it should live in state, with no mutability possibilities, passing around bits of it to the various services needing those. We aren't working in an object oriented programming environment, but rust has some traits from that system as well, so I think we should use everything we can to make our code better and easier to follow, and that includes the sound parts of OOP principles
That's why I think this should be done in main, if not the main function, at least the main module. You see, the configuration should be initialised first, because it initialises everything else, including logging. State::new already needs logging, for various things, especially because it then goes on and initialises lots of the core components of the screenreader, which in turn need logging, and orphan spans will be weird in the visualisers or even if we inspect the logs by hand, so that whole chain requires that logging is initialized before it.
|
Really liking this! Thank you for your time and effort! |
|
no problem at all! someone has to do these things as well |
…ilia this is currently using figment as the configuration library, other options include config and simply serde-toml this is not yet complete, as huge refactorings are about to take place
…ke state::new accept the configuration structure as a parameter this makes it easier to make, for example, speech have the desired rate from startup, logging be initialised from the config with the desired filter and perhaps a path location, stuff like that
the method join replaced the previously used merge. Apparently, with merge, when conflicts are encountered, it preferes keeping the current value, instead of replacing it. So, config was taken from the default values supplied with the modules, but the values in there were never replaced by those who should have a greatter priority. as a consequence, user defined configuration files wouldn't be applied, and the user would understandably be confused by the results
up to now, configuration was read, but never actually used. This begins a series of changes, perhaps across multiple fronts, to do so. This makes `state::new` send a message on the ssip channel, with the request to set the rate to a user defined value warning: if somehow the ssip task isn't initialized by the point we get there, the change will be lost in the channel, or may be picked up later than intended
we use nested toml tables in our configuration, one table per what we think to be a logical section. However, because we want to add environment variables as configuration sources, we have to be able to parse nested dictionaries. In order to do that, beside just using .prefix to filter out variables which don't concern us, we must also split the string of the variable name in keys, and for that we try to use the .split method
…ur composite configuration keys
…ation file and make the configuration struct easier to use
… for the system journal and tty * specify tty in the config file for logs to be sent to your terminal * use the file option to send it to a file * use syslog for writing to the journal, for systemd equipped distros
this makes the configuration system far more resiliant to problems, as well as introduces some composability to it. This is how configuration is sourced at the moment, feedback welcome:
if any of those files don't exist, nothing will happen, because a good known default set is provided at startup, anything else is in addition to that.
if a file in XDG_CONFIG_HOME doesn't exist, it will be created with the default configuration. I may move it to later in the chain, to populate it with values from all other config sources listed above instead of only the defaults