-
-
Notifications
You must be signed in to change notification settings - Fork 6.8k
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: Add getConfigValue
#5251
feat: Add getConfigValue
#5251
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## sidv/mindmapToTs #5251 +/- ##
=====================================================
- Coverage 80.08% 43.25% -36.84%
=====================================================
Files 167 23 -144
Lines 13867 5056 -8811
Branches 741 23 -718
=====================================================
- Hits 11106 2187 -8919
- Misses 2594 2868 +274
+ Partials 167 1 -166
Flags with carried forward coverage won't be shown. Click here to find out more.
|
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 wouldn't really recommend this approach. The path approach isn't type-safe since it's being used as a string. It's kind of complicating the object destructuring.
@@ -47,7 +47,7 @@ const addNode = (level: number, id: string, descr: string, type: number) => { | |||
descr: sanitizeText(descr, conf), | |||
type, | |||
children: [], | |||
width: conf.mindmap?.maxNodeWidth ?? defaultConfig.mindmap.maxNodeWidth, | |||
width: getConfigValue(conf, 'mindmap.maxNodeWidth'), |
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'm also not a big fan of this syntax, since it makes things more complicated.
Ideally, we'd instead change getConfig()
to return RequiredDeep<MermaidConfig>
.
The only reason I haven't yet is that some of the config values don't have default values, because undefined
is a valid value for them. Ideally, I'd go through the config and change every value that currently accepts undefined
to instead use null
, that way we can safely do RequiredDeep<MermaidConfig>
and still keep the null
types.
Then we could just do: conf.mindmap.maxNodeWidth
.
path: Path | ||
): Get<RequiredDeep<MermaidConfig>, Path> => { | ||
let value = lodashGet(config, path) as Get<RequiredDeep<MermaidConfig>, Path>; | ||
if (!value) { |
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.
This doesn't work for values that valid but falsy, false
, 0
, or ""
.
No description provided.