Feature/action context #183
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Pull Request Test Coverage Report for Build 326
💛 - Coveralls |
|
I prefer 2 or 4. But 2 prevents defining contexts in config files, so I would keep it at 4. The majority of the actors doesn't make use of the context anyway, and if someone creates an actor that makes a lot of use of the context they can define their own variables to make for easier development. |
|
Ok, just added backwards compatibility. Ready for review now. |
rubensworks
force-pushed
the
feature/action-context
branch
from
2b97c7f
to
5aa0c69
Compare
joachimvh
approved these changes
Jul 26, 2018
| * @return {Promise<T>} A promise that resolves to the handle test result. | ||
| */ | ||
| public abstract async testHandle(action: HI, mediaType: string): Promise<HT>; | ||
| public abstract async testHandle(action: HI, mediaType: string, context?: ActionContext): Promise<HT>; |
There was a problem hiding this comment.
Should the context parameter be optional? This might encourage developers to not pass on the context when they receive it.
There was a problem hiding this comment.
Yes, that makes sense. The context can still be null, but all remove the ?everywhere to make it clear that the context must be passed along.
This is an abstraction of the query operation context, and allows data to be passed to any actor. Closes #109
rubensworks
force-pushed
the
feature/action-context
branch
from
5aa0c69
to
52be168
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Implements the generalization of a shared context #109.
Do not merge or review yet.Context keys are now namespaced, for example:
{ '@comunica/bus-rdf-resolve-quad-pattern:sources': [{ type: 'hypermedia', value: ... }] }The problem with this is that it becomes less convenient for programmatic API users to define sources.
A few possible solutions:
newSourcesContext([...]). Downside is that it becomes more complicated when your context consists of several other stuff as well.@joachimvh What is your opinion on these solutions?.