t/16: Introduced the configuration API #19
Conversation
| get: function( name ) { | ||
| // The target for this configuration is, for now, this object. | ||
| //jscs:disable safeContextKeyword | ||
| var source = this; |
There was a problem hiding this comment.
Why we need this variable? And why it's sometimes called source and sometimes target? I suggest to follow common approach and call them that or self .
There was a problem hiding this comment.
that is a trick to reference this inside inner-functions. That's not the case here.
You should read the rest of the code... here source (on get()) and target (on set()) refer to the source/target object for the configurations, which at start is this but then eventually moves to child properties of this. Therefore a more semantical name is used for the variable and unfortunately the jscs rules we have applied are too strict for this.
There was a problem hiding this comment.
y, it make sense - good naming.
| * @param {*} [value] The configuration value. Used if a name is passed to nameOrConfigurations. If undefined, | ||
| * the configuration is set to `null`. | ||
| */ | ||
| define: function( name, value ) { |
There was a problem hiding this comment.
I'm not really convinced to the name of this method... Wouldn't it make more sense to name it default and store its values in this.defaults?
There was a problem hiding this comment.
I'm out of context right now so ignore my message if it misses the point. But I remember that defining config properties was aimed at not only defining defaults, but also what configuration options are out there (possible validation).
There was a problem hiding this comment.
aimed at not only defining defaults, but also what configuration options are out there (possible validation)
Exactly... that's why it's named "define".
|
Ok, minor fixes applied. Ready for review again. |
|
What about all those assertions we discussed earlier? Are you gonna address them now or while resolving #20? |
Let's do all in a roll right after closing this PR, if you don't mind. |
|
Alright, so if there is no more comments, then I'm OK with closing this one. |
t/16: Introduced the configuration API
Fixes ckeditor/ckeditor5#3434.