-
Notifications
You must be signed in to change notification settings - Fork 6
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
Add support for list overrides #134
Conversation
Pull Request Test Coverage Report for Build 7061626539
💛 - Coveralls |
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 is a really nice PR, I like it!
Just one minor comment, everything else looks perfect IMO.
Before we merge, could you do a quick check to see if it doesn't break anything in Comunica? (I suspect it won't, since Comunica doesn't use overrides anywhere in any case)
|
||
public constructor(options: IComponentConfigPreprocessorOverrideOptions) { | ||
this.objectLoader = options.objectLoader; | ||
this.componentResources = options.componentResources; | ||
this.logger = options.logger; | ||
|
||
this.stepHandlers = [ |
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.
Instead of adding the handlers here, could we pass them through the options
from ComponentsManagerBuilder
?
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.
No strong opinion either way, just that this was based on how this is done in some other classes:
Components.js/lib/preprocess/ConfigPreprocessorComponentMapped.ts
Lines 39 to 45 in d33d4c2
private readonly mappingHandlers: IConstructorArgumentsElementMappingHandler[] = [ | |
new ConstructorArgumentsElementMappingHandlerKeyValue(), | |
new ConstructorArgumentsElementMappingHandlerCollectEntries(this.parameterHandler), | |
new ConstructorArgumentsElementMappingHandlerFields(), | |
new ConstructorArgumentsElementMappingHandlerElements(), | |
new ConstructorArgumentsElementMappingHandlerList(), | |
]; |
Components.js/lib/preprocess/ParameterHandler.ts
Lines 21 to 27 in a553310
this.parameterPropertyHandlers = [ | |
new ParameterPropertyHandlerDefaultScoped(this.objectLoader), | |
new ParameterPropertyHandlerDefault(this.objectLoader), | |
new ParameterPropertyHandlerFixed(this.objectLoader), | |
this.parameterPropertyHandlerRange = new ParameterPropertyHandlerRange(this.objectLoader, options.typeChecking), | |
new ParameterPropertyHandlerLazy(), | |
]; |
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.
Oh, I see. In that case, you can ignore my comment.
Everything seems to work fine when testing with Comunica. |
Released as 5.5.0. |
Closes #129
Implements what is described in #129 (comment)
Will look into documentation when this is accepted.
Some of the behaviour that happened in the
canHandle
of theConfigPreprocessorOverride
was moved to thetransform
call, but this should not have an impact.