Skip to content

feat/refactor: prevent config deletion when a new field is added#131

Closed
lorenzoberts wants to merge 5 commits intokworkflow:unstablefrom
lorenzoberts:lorenzo/issue-118
Closed

feat/refactor: prevent config deletion when a new field is added#131
lorenzoberts wants to merge 5 commits intokworkflow:unstablefrom
lorenzoberts:lorenzo/issue-118

Conversation

@lorenzoberts
Copy link
Collaborator

This PR aims to solve issue #118 .

The first commit is a minor refactor to improve code quality in the app/config.rs module. It removes redundancies, centralizes the config file name construction and implements the std::Default trait (the default function was already there; i just had to move it to the trait impl).

The second commit solves the issue itself, but without any "optimizations". As @davidbtadokoro mentioned in the issue description, we just had to create a #[serde(default = "")] attribute for each Config field. We could merge only the first two commits, and the main problems would have been solved. Despite that, with this solution we've created two other problems:

  1. For each field's default function, we need to reinitialize the Config::default() object. This means accessing env vars and creating strings from scratch for every field that cannot be deserialized automatically.
  2. For each new field we add to Config, we would have to also add the serde(default = "") attribute and the default implementation, which is easy but not quite clean, making the structure hard to read and the config.rs code too extensive.

The third commit solves problem 1 by applying lazy_static macro, which creates the Config::default object only once at runtime.

The fourth commit solves problem 2 by creating a procedural macro, which will do the exact same thing commit 2 and 3 (combined) did, but it is extensible for all eventual new fields the struct Config will receive.
I found it a good opportunity to create the first procedure macro for patch-hub but, as any macro, it only solves developers' pains, so if you think it's overkill we can stop the issue resolution on commit 3.

The fifth commit creates unit tests for the procedural macro and it changes the CI workflow so it is can run those tests.

This commit improves code quality in config.rs module by
1. implementing std::Default instead of using a loose fn default()
2. creating a get_config_path function, which centralizes config path build
and therefore helps remove duplicate code.
3. changing build config logic to save Config struct back to config file
right after loading it.

The improvement number 3 was done in case loaded Config struct is somehow different
from the saved one. This will be crucial for solving issue kworkflow#118.

Signed-off-by: Lorenzo Bertin Salvador <lorenzobs@usp.br>
… created

This solves kworkflow#118 by creating individual default functions for each Config
attribute, so if I new attribute is created, an existing config file
will not fail to serialize, it will just create the new attribute with
its default value.

Signed-off-by: Lorenzo Bertin Salvador <lorenzobs@usp.br>
…in memory

This is the first optimization for Config individual attributes serialization.
With this, we don't need to create a new Config object each time we access
the default attribute functions (normally in deserialization).

Signed-off-by: Lorenzo Bertin Salvador <lorenzobs@usp.br>
This commit adds a procedural macro to patch-hub! The intention is
to simplify the Config struct by avoiding to add a #[serde(default = "")]
for each of its attributes and implement the default function.
The proc_macro supports all kinds of structs and should preserve
all other struct attributes, derivations and general behavior.
This completely solves kworkflow#118.

Signed-off-by: Lorenzo Bertin Salvador <lorenzobs@usp.br>
This commit adds unit tests for the serde_individual_default procedural
macro. As a consequence, it is necessary to add the flag --all on
cargo test so the new proc_macro tests are executed.

Signed-off-by: Lorenzo Bertin Salvador <lorenzobs@usp.br>
@lorenzoberts lorenzoberts changed the title feat/refactor: Prevent config deletion when a new field is added feat/refactor: prevent config deletion when a new field is added Mar 23, 2025
@davidbtadokoro
Copy link
Collaborator

davidbtadokoro commented Apr 7, 2025

Hey @lorenzoberts, and thank you so much for this PR! I have a list of things to say, so here are some bullet points:

  • It indeed solves issue patch-hub config file gets deleted when adding new field to Config #118. Tested it through and through, and it didn't break either in intermediary commits;
  • I loved how you didn't stop in solving the issue, but also thought about the maintainability of your solution. I had to spend some time in learning procedural macros to adequately review this PR, which is good for knowledge, and also made me agree with this approach to avoid having to have a lot of boilerplate and annoyance when reading/changing app/config.rs (or future modules with the same behavior);
  • All commits were perfect, IMHO, and I had virtually no tinkers, besides a preliminary commit (next bullet). Kudos for the messages and clean implementation (I struggled a little with the macro, but it turned out to be really clear 😺).
  • Because we are now altering the config file when calling Config::build() - which I agree to enforce consistency of the loaded and consolidated files - we had a side-effect of altering the sample config file src/test_samples/app/config/config.json. This makes running the test suite alter this git tracked file, so I added a preliminary commit to make the test suite use a temporary sample config file (with the same contents as the tracked one) to avoid this and not change your solution.

Long story short, thank you so much for this marvelous PR, and sorry for the delay. Change merged into the unstable branch 👍

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.

2 participants