-
Notifications
You must be signed in to change notification settings - Fork 10
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
feat: Read rest of options from custom configuration file #391
feat: Read rest of options from custom configuration file #391
Conversation
export type CustomConfig = { | ||
includes?: string[]; | ||
output?: string; |
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.
Re-ordering in some places so the options are in the same order as in the documentation for consistency and readability.
src/commands/generate.ts
Outdated
@@ -97,39 +97,34 @@ export const generate: Generate = async ({ rootDir, includes, useMaintainers = f | |||
} | |||
}; | |||
|
|||
interface Options { | |||
output?: string; | |||
interface Options extends GlobalOptions { | |||
verifyPaths?: boolean; |
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.
Not quite related, but is verifyPaths
still needed? Only exists in this file, so can it be removed?
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.
As far as I can see, looking at the commit history, verifyPaths
has never been used. So, it can safely be removed.
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.
Have removed in 53ecf24
|
||
const { output = globalOptions.output || OUTPUT } = options; | ||
const output = options.output || globalOptions.output || OUTPUT; |
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 find this syntax more readable as it only uses one technique
Pull Request Test Coverage Report for Build 7475666855
💛 - Coveralls |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #391 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 11 11
Lines 284 287 +3
Branches 64 67 +3
=========================================
+ Hits 284 287 +3
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.
I will wait today to merge it so @gustavkj can swing by review again. If not, I will release it on Friday. Thank you
Problem
Not all of the options that can be set through CLI flags are able to be read in from the custom config file. These being:
useMaintainers
useRootMaintainers
preserveBlockPosition
It would improve dev experience to also set these in the custom config and have them be respected like the other options.
Changes
generate
to read in the options mentioned above from custom configCustomConfig
,GlobalOptions
andOptions
types