-
-
Notifications
You must be signed in to change notification settings - Fork 301
feat(cli): add config option to specify config file path #1047
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
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@ ## master #1047 +/- ## ========================================== + Coverage 97.33% 97.50% +0.16% ========================================== Files 42 55 +13 Lines 2104 2446 +342 ========================================== + Hits 2048 2385 +337 - Misses 56 61 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
What's the motivation for this change?
What's the motivation for this change?
Simply just found out #656 is quite interesting for me,
but actually I can not imagine about the usage or in what scenario user will use this option.
What's the motivation for this change?
Simply just found out #656 is quite interesting for me, but actually I can not imagine about the usage or in what scenario user will use this option.
Someone who wants to use a single config for all projects might need it. or mono repo maybe?
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.
Hi @rockleona, thanks so much for implementing this. I left a few comments.
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.
I think we're really close to merging! In addition to the comment to address, I would suggest you squash the latest 2 commits into the first one as they're minor fixes and do not really need to appear in the changelog. Thanks!
cee220c to
bf6a6bf
Compare
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.
LGTM. @woile @noirbizarre I'll merge this one these days. Please let me know if you want to take a deeper look
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.
Good to me.
Non blocking suggestion: test and ensure that user home path expansion works (users WILL use home-relative forms for sure so handling cases like ~/path/to/config from start will avoid inevitable issues)
Good to me.
Non blocking suggestion: test and ensure that user home path expansion works (users WILL use home-relative forms for sure so handling cases like
~/path/to/configfrom start will avoid inevitable issues)
Tested on my end. Works fine. Gonna merge it 🚀
Uh oh!
There was an error while loading. Please reload this page.
Description
CLI now have an option
--configcan specify the config file path,if the config file is not in root folder, user can specify the located file as well.
Checklist
./scripts/formatand./scripts/testlocally to ensure this change passes linter check and testExpected behavior
cznow have an option to specify config file,if the given file is existed, will only read configuration from the given file,
else, a error would be raise since the given file do not exist.
Steps to Test This Pull Request
Execute any
czcommand with--configoption, if the config file do not exist, a error would raised.Additional context
resolve #656