Feature/memento#195
Conversation
|
|
|
Something seems to be wrong with the lockfile. Maybe try forcefully checking the lockfile out from master and re-running Also, be sure to sign the CLA. |
Pull Request Test Coverage Report for Build 383
💛 - Coveralls |
rubensworks
left a comment
There was a problem hiding this comment.
It looks like the test coverage for init-sparql and the memento actor is not 100% yet.
| public async run(action: IActionHttp): Promise<IActorHttpOutput> { | ||
| const datetime: Date = action.context && action.context.get('datetime'); | ||
|
|
||
| // 1. Create ActionHttp |
There was a problem hiding this comment.
I would remove these 1., 2., 3. 4., 5. comments, they don't really add any information next to the code IMO.
| status: httpResponse.statusCode, | ||
| url: httpResponse.responseUrl, | ||
| // when the content came from another resource because of conneg, let Content-Location deliver the url | ||
| url: headers.has('content-location') ? headers.get('content-location') : httpResponse.responseUrl, |
There was a problem hiding this comment.
Can you also add a dedicated unit test for this to the HTTP-native actor?
|
@rubensworks I've tried to cover the requested changes as much as possible, but for the unit tests it takes me to long to figure out the 100% coverage in |
|
According to the coveralls logs this is the line that is missed: https://coveralls.io/builds/18360561/source?filename=packages%2Factor-init-sparql%2Flib%2FActorInitSparql.ts#L105 And here is the coverage file for the memento actor: https://coveralls.io/builds/18360561/source?filename=packages%2Factor-http-memento%2Flib%2FActorHttpMemento.ts#L23 (You should be able to see the non-covered lines when running |
|
Yeah of course, I got that. Let me be more specific:
|
|
So for actor-init-sparql, you can add a test that executes the command with the For the memento actor, you should add a test that has a context, but does not have an |
|
it should be fine now |
rubensworks
left a comment
There was a problem hiding this comment.
Oh wait, just noticed on minor thing.
|
|
||
| // Define the datetime | ||
| if (args.d) { | ||
| context.datetime = new Date(args.d); |
There was a problem hiding this comment.
I just noticed this, but this has to be set via a namespaced context key, just like KEY_CONTEXT_SOURCES below. You could add something like KEY_CONTEXT_DATETIME to the memento actor class.
There was a problem hiding this comment.
ActorHttpMemento also has to be updated to use this key, it will always fail otherwise, as well as the unit tests.
|
I wonder whether we should add more documentation in the README.md about how this actor could be invoked from different query languages, as a library or as a command line interface. Right now, only the pull request mentions a |
|
@pietercolpaert I think this is a problem in general for all actors. At some point we should start working on some detailed usage documentation for each actor, see #116. |
This implements the Memento protocol (#79) and adds an
-dparameter to theactor-init-sparql.