Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(config): refactor the config system #62

Merged
merged 45 commits into from
Jul 11, 2023

Conversation

0x61nas
Copy link
Contributor

@0x61nas 0x61nas commented Jun 27, 2023

Description

Make the Config the main settings container instead of CliArgs. and auto-create the config file in ~/.confg/halp/config.toml if it doesn't exist, and give the command-line arguments priority over the config file

Motivation and Context

How Has This Been Tested?

  • By running cargo t --lib, all the tests are passed
  • By run cargo r -- --help and cargo r -- plz --help, it gives me the expected output
  • By using the --no-version and --no-help flags

Types of Changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation (no code change)
  • Refactor (refactoring production code)
  • Other

Checklist:

  • My code follows the code style of this project.
  • I have updated the documentation accordingly.
  • I have formatted the code with rustfmt.
  • I checked the lints with clippy.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@0x61nas 0x61nas marked this pull request as ready for review June 27, 2023 15:41
@0x61nas 0x61nas requested a review from orhun as a code owner June 27, 2023 15:41
@codecov-commenter
Copy link

codecov-commenter commented Jun 27, 2023

Codecov Report

Patch coverage: 76.75% and project coverage change: -2.41 ⚠️

Comparison is base (bed5192) 80.85% compared to head (94a6685) 78.43%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #62      +/-   ##
==========================================
- Coverage   80.85%   78.43%   -2.41%     
==========================================
  Files          14       14              
  Lines         595      598       +3     
==========================================
- Hits          481      469      -12     
- Misses        114      129      +15     
Impacted Files Coverage Δ
src/error.rs 100.00% <ø> (ø)
src/helper/docs/mod.rs 0.00% <0.00%> (ø)
src/lib.rs 50.00% <31.25%> (-15.00%) ⬇️
src/config.rs 75.37% <72.10%> (-14.80%) ⬇️
src/cli.rs 83.34% <92.11%> (+11.34%) ⬆️
src/helper/args/mod.rs 97.06% <96.16%> (-1.10%) ⬇️
src/helper/docs/cheat.rs 70.22% <100.00%> (+0.65%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@0x61nas 0x61nas changed the title refactor(configs): Move the update_args function from the Config struct into the CliArgs refactor(config): Make the Config struct the main setings container Jun 27, 2023
@0x61nas 0x61nas marked this pull request as draft June 27, 2023 18:33
@0x61nas 0x61nas marked this pull request as ready for review June 27, 2023 20:07
@0x61nas 0x61nas changed the title refactor(config): Make the Config struct the main setings container refactor(config): Make the Config struct the main settings container and auto-create the config file if it doesn't exist Jun 27, 2023
@0x61nas
Copy link
Contributor Author

0x61nas commented Jul 2, 2023

I messed up the commit history a little bit in my attempt to fix the typo in 2f2ccc0 commit message, sorry about that

@0x61nas 0x61nas requested a review from orhun July 3, 2023 14:03
Copy link
Owner

@orhun orhun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! 🐻

@orhun orhun changed the title Refactor the config system refactor(config): refactor the config system Jul 11, 2023
@orhun orhun merged commit a639fa9 into orhun:main Jul 11, 2023
@orhun
Copy link
Owner

orhun commented Jul 11, 2023

Hey @0x61nas I just realized --check doesn't work after this, can you check?

@0x61nas
Copy link
Contributor Author

0x61nas commented Jul 12, 2023

Oh, I didn't notice that. I'll fix this issue quickly as I can

@0x61nas 0x61nas deleted the refactor-configs-code branch July 12, 2023 06:30
This was referenced Jul 12, 2023
bors bot added a commit that referenced this pull request Jul 17, 2023
81: fix(cli): fix the check option  r=orhun a=0x61nas

<!--- Thank you for contributing to halp! 🐙 -->

## Description
Add the missing code to override the check args in the config instance with the `--check` CLI option

## Motivation and Context
- Fix the issue that's introduced in #62 

## How Has This Been Tested?
- I trayed to pass `--check="-h"` and it worked as expected
- I trayed to pass ` --check='-h' --check='-V'` and it worked as expected also

## Types of Changes
- [x] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to change)
- [ ] Documentation (no code change)
- [ ] Refactor (refactoring production code)
- [ ] Other <!--- (provide information) -->

## Checklist:
- [x] My code follows the code style of this project.
- [x] I have updated the documentation accordingly.
- [x] I have formatted the code with [rustfmt](https://github.com/rust-lang/rustfmt).
- [x] I checked the lints with [clippy](https://github.com/rust-lang/rust-clippy).
- [x] I have added tests to cover my changes.
- [x] All new and existing tests passed.

> This's an alternative to #79 

Co-authored-by: Anas Elgarhy <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants