xline-kv / Xline

A geo-distributed KV store for metadata management
https://xline.cloud
Apache License 2.0
565 stars 71 forks source link

refactor: split config into multiple modules #676

Open Harsh1s opened 4 months ago

Harsh1s commented 4 months ago
Harsh1s commented 4 months ago

I'll refactor the new methods into builder methods into another commit. Before doing that just some wanted feedback if possible.

mergify[bot] commented 2 months ago

@Harsh1s Convert your pr to draft since CI failed

mergify[bot] commented 2 months ago

@Harsh1s Your PR is in conflict and cannot be merged.

Phoenix500526 commented 1 month ago

Hi, @Harsh1s ! This pr has been stalled for 2 months. Would you like to update it? 😄

Harsh1s commented 1 month ago

Hi, @Harsh1s ! This pr has been stalled for 2 months. Would you like to update it? 😄

I am so very sorry, I assure you I'll update it within a day.

Harsh1s commented 1 month ago

@Phoenix500526 I redid the refactor with latest changes, I still have to rewrite new methods into builder pattern which I'll do by EOD. Could you please review if current refactor is fine? I'll do the builder rewrite in the meantime.

mergify[bot] commented 1 month ago

@Harsh1s Convert your pr to draft since CI failed

codecov[bot] commented 1 month ago

Codecov Report

Attention: Patch coverage is 89.13325% with 84 lines in your changes are missing coverage. Please review.

Project coverage is 75.54%. Comparing base (e35b35a) to head (3482952). Report is 84 commits behind head on master.

:exclamation: Current head 3482952 differs from pull request most recent head 93f0429

Please upload reports for the commit 93f0429 to get more accurate results.

Files Patch % Lines
crates/utils/src/config/metrics.rs 57.62% 22 Missing and 3 partials :warning:
crates/utils/src/config/log.rs 60.37% 17 Missing and 4 partials :warning:
crates/utils/src/config/compact.rs 57.57% 11 Missing and 3 partials :warning:
crates/utils/src/config/curp.rs 85.93% 7 Missing and 2 partials :warning:
crates/utils/src/config/client.rs 95.16% 1 Missing and 2 partials :warning:
crates/utils/src/config/cluster.rs 94.44% 0 Missing and 3 partials :warning:
crates/utils/src/config/mod.rs 99.40% 1 Missing and 1 partial :warning:
crates/utils/src/config/server.rs 95.12% 0 Missing and 2 partials :warning:
crates/utils/src/config/auth.rs 85.71% 0 Missing and 1 partial :warning:
crates/utils/src/config/engine.rs 75.00% 0 Missing and 1 partial :warning:
... and 3 more
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #676 +/- ## ========================================== - Coverage 75.55% 75.54% -0.02% ========================================== Files 180 198 +18 Lines 26938 27496 +558 Branches 26938 27496 +558 ========================================== + Hits 20353 20771 +418 - Misses 5366 5446 +80 - Partials 1219 1279 +60 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Phoenix500526 commented 1 month ago

Hi, @Harsh1s I've rebased this pr and modify the code to pass the ci process.

Harsh1s commented 1 month ago

Thanks for the help @Phoenix500526 , I've pulled the changes and I'll address all your reviews now. I also started working on builder patterns for all the structs, I'll make commits one by one.

mergify[bot] commented 1 month ago

@Harsh1s Convert your pr to draft since CI failed

Phoenix500526 commented 1 month ago

Hi, @Harsh1s! IMO, to implement the builder pattern for all these XXConfig structures may not be a good practice. I only recommend implementing the builder pattern for those XXConfig structures with too many arguments in their constructor. BTW, to pass the commit message validation ci task, please use "refactor(config)" or "refactor" to replace the "refactor-config" in your commit message. 😄

Harsh1s commented 1 month ago

Hi, @Harsh1s! IMO, to implement the builder pattern for all these XXConfig structures may not be a good practice. I only recommend implementing the builder pattern for those XXConfig structures with too many arguments in their constructor. BTW, to pass the commit message validation ci task, please use "refactor(config)" or "refactor" to replace the "refactor-config" in your commit message. 😄

Understood, thank you so much for all the help and being so patient with me!

Phoenix500526 commented 1 month ago

We also provide the .pre-commit-config.yaml in our project root. You can execute the pre-commit install command at the project root to install the git hook. It will automatically run the cargo fmt, cargo clippy and validate the commit message for you before you commit your code modifications. I think it's helpful.

Harsh1s commented 1 month ago

We also provide the .pre-commit-config.yaml in our project root. You can execute the pre-commit install command at the project root to install the git hook. It will automatically run the cargo fmt, cargo clippy and validate the commit message for you before you commit your code modifications. I think it's helpful.

Okay, will do. Thanks!