Closed alexdewar closed 4 months ago
Attention: Patch coverage is 59.09091%
with 27 lines
in your changes missing coverage. Please review.
Project coverage is 65.62%. Comparing base (
387fd3b
) to head (f00b7d0
). Report is 4 commits behind head on main.
Files | Patch % | Lines |
---|---|---|
src/settings.rs | 72.22% | 15 Missing :warning: |
src/main.rs | 0.00% | 8 Missing :warning: |
src/log.rs | 0.00% | 3 Missing :warning: |
src/simulation.rs | 0.00% | 1 Missing :warning: |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
@dalonsoa I've had another go, addressing your comments.
Now users can either set the logging level via an environment variable (MUSE2_LOG_LEVEL
) or the settings.toml
file, with the environment variable taking precedence (I figure it's probably handy to have this for debugging). As we want to be able to use the program logger to log progress as we load the various input files, we need to initialise the logger after reading settings.toml
(so we know if the user has configured the log level or not), but before loading any of the CSV files. One option would have been to initialise the logger in the read_settings()
function, but that seemed like a poor separation of concerns. Instead, I refactored the code so that reading the TOML file and reading the other files are carried out in two steps; in between them, we can extract the log level and set it. I've refactored settings.rs
using a more idiomatic Rust style. Now the struct with the raw input data from settings.toml
is called SettingsReader
. It provides a method for accessing the log level and for converting it into a Settings
struct, which is the higher-level representation of the model settings. I also followed a suggestion you made somewhere else about reading the settings in main()
, then passing them as an argument to simulation::run()
, which I think is cleaner.
Yeah :disappointed:. Part of the issue is that mocking in Rust is really hard, but also I've just been a bit lazy :stuck_out_tongue:. Some of the bits that are untested were untested before though, it's just that codecov is flagging them again because of the refactoring.
Honestly I'm not sure what the best approach is for testing in Rust. For little functions, it's easy enough to write a unit test, but for higher-level functions that call these little functions, it's hard to think of a sensible test. I guess maybe we just write functional tests for these ones?
Yeah, I think rust
encourages write code in small chunks that are easy to test. Anything bigger than that, looks more like an integration test.
Description
For various reasons, it's better to use a proper logging library rather than having print statements dotted throughout the code.
I've added
env_logger
as a dependency (it seems to be a conservative choice) and used it in a few places in the code. The default output will look something like this:We can always configure it later if we want it to look different.
Closes #91.
Type of change
Key checklist
$ cargo test
$ cargo doc
Further checks