set autostart to false #36
Conversation
| if (removeOptions) | ||
| options = undefined; | ||
| } | ||
|
|
||
| // Set transformation steps from the options | ||
| options = options || (!isSourceExpression(source) ? source : null as any); |
There was a problem hiding this comment.
This was the only cause of some problems in the testing - since passing option = { autoStart: true } means that the source is not used even if it is not a source expression.
I've got around this by setting the options to undefined if the only key is autoStart - but I'm thinking that the tests should just be changed instead
There was a problem hiding this comment.
I'm sorry, I've been looking at this for 15 minutes now but still don't understand.
So the logic of the SimpleTransformIterator is that both of its parameters (source and options) are optional.
So what the original line 1224 is doing is determining whether options is the first or the second argument (given that source is optional, it might change position). The easy case is when options is non-nullish: then options is the second argument. If options is nullish, then options is the first argument—if that first argument is not a source.
None of that should change, regardless of the autoStart default. So probably a test change indeed.
There was a problem hiding this comment.
Makes sense - will do a test change then :)
test/ArrayIterator-test.js
Outdated
| describe('An ArrayIterator undefined iterator and autostart', () => { | ||
| let iterator; | ||
| before(() => { | ||
| iterator = new ArrayIterator(); | ||
| iterator = new ArrayIterator(undefined, { autoStart: true }); |
There was a problem hiding this comment.
That is a bit weird to me; I'll investigate.
There was a problem hiding this comment.
Was using undefined to emulate the existing test with no sources. An empty array should work as well instead.
test/BufferedIterator-test.js
Outdated
| describe('A BufferedIterator without arguments', () => { | ||
| let iterator; | ||
| before(() => { | ||
| iterator = new BufferedIterator(); | ||
| iterator = new BufferedIterator({ autoStart: true }); |
There was a problem hiding this comment.
This is where this change will get a bit annoying, but I think it's important that the test structure remains the same. So first "without arguments" (so don't pass anything) and then the other cases.
(Note that the description and code are out of sync too.)
There was a problem hiding this comment.
Looking good!
I think we just need to add autoStart to all implementations now. So basically any class deriving from AsyncIterator (except from AsyncIterator itself, it seems).
Or, instead of adding the autoStart option, we need to change the behavior of SingletonIterator to just not automatically start (which is now the default).
| describe('An ArrayIterator undefined iterator and autostart', () => { | ||
| let iterator; | ||
| before(() => { | ||
| iterator = new ArrayIterator(undefined, { autoStart: true }); |
There was a problem hiding this comment.
| describe('An ArrayIterator undefined iterator and autostart', () => { | |
| let iterator; | |
| before(() => { | |
| iterator = new ArrayIterator(undefined, { autoStart: true }); | |
| describe('An ArrayIterator without source with autostart', () => { | |
| let iterator; | |
| before(() => { | |
| iterator = new ArrayIterator({ autoStart: true }); |
(should be made to work if it doesn't)
| { autoStart = false, ...options }: BufferedIteratorOptions = {}) { | ||
| super({ autoStart, ...options }); |
There was a problem hiding this comment.
I wouldn't deconstruct here; we can just use options.autoStart below. The original was needed because we didn't want to repeat the !== false cast, which is no longer needed (as in 6f3af33).
| expect(iterator.read()).to.be.null; | ||
| }); | ||
| }); | ||
|
|
||
| describe('An ArrayIterator with an empty array without autoStart', () => { |
There was a problem hiding this comment.
| describe('An ArrayIterator with an empty array without autoStart', () => { | |
| describe('An ArrayIterator with an empty array', () => { |
In order to handle #35 (comment) both solutions essentially are determining whether or not to autoStart the Also FYI @rubensworks with the current status of the PR only 15 tests from 7 test suites fail in the whole of comunica. Most of these issues arise from |
I'm fine with either option, whatever is easiest. |
No description provided.