-
Notifications
You must be signed in to change notification settings - Fork 154
fix(plugins.js) : makes the config parameter 'withHeadings' works when data is empty #134
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
base: master
Are you sure you want to change the base?
Conversation
baf0ec5 to
412e409
Compare
src/plugin.js
Outdated
| if (data) { | ||
| return data[configName] ? data[configName] : defaultValue; | ||
| } | ||
| if (data && configName in data) return data[configName] |
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.
| if (data && configName in data) return data[configName] | |
| if (data && configName in data) { | |
| return data[configName] | |
| } |
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 accepted this change (maybe adding a prettier to the project could avoid this kind of formatting stuffs)
| if (data && configName in data) return data[configName] | ||
|
|
||
| return this.config && this.config[configName] ? this.config[configName] : defaultValue; | ||
| return this.config && this.config[configName] ? this.config[configName] : false; |
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.
it will always return false event when there is no value in the config. I think it is not what is expected here. Why default value have been 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.
Originally, getConfig() was called with defaultValue = false, as you can see here: this.getConfig('withHeadings', false, data)
getConfig(configName, defaultValue = undefined, savedData = undefined) {
const data = this.data || savedData;
if (data) {
return data[configName] ? data[configName] : defaultValue;
}
return this.config && this.config[configName] ? this.config[configName] : defaultValue;
}
So the result remains the same: false is returned when none of the conditions are met (so technically there is still a default value, it has not been removed).
To provide more context:
I don't really understand the point of having a defaultValue parameter defaulting to undefined (notably because we'd like getConfig() to return a clear value for withHeadings - which should be a boolean according to the documentation) .
And since:
getConfig()is not used elsewhere- With the update I made to
getConfiga default value is needed only once (versus 2 before)
I just get rid of defaultValue and made getConfig directly return false if none of the conditions are met.
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.
@neSpecc Any feedback on this issue?
412e409 to
23a3635
Compare
| */ | ||
| getConfig(configName, defaultValue = undefined, savedData = undefined) { | ||
| getConfig(configName, savedData = undefined) { | ||
| const data = this.data || savedData; |
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.
To achieve fixing config, I suggest not using this.data because getConfig is being used for making this.data.
and it works in my case.
| const data = this.data || savedData; | |
| const data = savedData; |
Noticed that the config parameter
withHeadingswas not properly working.More precisely:
was not creating an empty table with headings.
So I made a fix. Tried to change as little code as possible (but I believe
getConfigshould be optimized further)This PR #126 also tackles the issue