feat/refactor: prevent config deletion when a new field is added#131
Closed
lorenzoberts wants to merge 5 commits intokworkflow:unstablefrom
Closed
feat/refactor: prevent config deletion when a new field is added#131lorenzoberts wants to merge 5 commits intokworkflow:unstablefrom
lorenzoberts wants to merge 5 commits intokworkflow:unstablefrom
Conversation
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>
Collaborator
|
Hey @lorenzoberts, and thank you so much for this PR! I have a list of things to say, so here are some bullet points:
Long story short, thank you so much for this marvelous PR, and sorry for the delay. Change merged into the unstable branch 👍 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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:
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.