-
-
Notifications
You must be signed in to change notification settings - Fork 32
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
chore: Allow partial configuration file #116
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for picking this up! 💖
I just had some comments for improvement.
Take a look and let me know what you think, by making those both Option, it did add some complexity when setting the default values but I added the two tests, one for each section, with the opposing one missing and then tested in both cases for each one. Idk what's going on with CI but I made changes it requested. Fsr clippy didn't give the same warnings when ran locally with the same command. |
Codecov ReportPatch coverage:
❗ 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 @@
## master #116 +/- ##
==========================================
- Coverage 62.19% 62.08% -0.10%
==========================================
Files 27 27
Lines 2240 2260 +20
==========================================
+ Hits 1393 1403 +10
- Misses 847 857 +10
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, overall it looks good. I just pushed e89b2be for cleanup.
Also I have 2 questions, then we can merge 🐻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution!
Description
This PR allows for the mandatory args that need to be generated from the config file, if the config file is found, be filled in with default arguments should they not be included in the found config file.
Please let me know if I missed anything, and thank you for the awesome program 👍
Motivation and Context
closes #105
How Has This Been Tested?
I tested this locally on my machine by placing a config file in ~/.config/gpg-tui and
filling adding and removing parts to make sure the program will still start.
I also wrote a test
test_args_partial_config
and included a partial conifg filegpg-tui-part
in the config directory of the repo, and ran assertions for the missing arguments.Output (if appropriate):
The program will still start, despite fields missing in the configuration file.
PREV:
AFTER:
Well it works 👍 a screenshot of the program running is probably overkill.
Types of changes
Checklist: