Wrap Iterator + Countable in CountableProviderRecord

[a-barzanti]

I've tried to create a resource with a $connector->fetch that returned a ArrayIterator (Countable) but the result was not an instance of Countable.
I solved using directly:
return new CountableProviderRecords($data, count($data), $this);
in the fetch method.
I thought if you pass an instance of countable this can be done directly by the import function.

[coveralls]

Coverage decreased (-0.2%) to 98.152% when pulling 0499d67 on a-barzanti:countable-iterator into c7c6d8a on ScriptFUSION:master.

[Bilge]

I don't understand the use-case for this. What object type are you returning from the provider resource that is both an Iterator and Countable but isn't already CountableProviderRecords, and why?

---

Edit: I apologise, I didn't read properly. I now understand you are returning ArrayIterator from the resource. However, why is this more convenient than just returning CountableProviderRecords instead?

[Bilge]

After some consideration, even without knowing your exact use-case, I think this is a good change. If we auto-wrap ProviderRecords why not do the same for CountableProviderRecords. However, Coveralls has reported coverage decrease indicating the new code is not tested.

In addition to adding tests, I suggest moving the wrapping code to a new private method since it has become more complex and isn't directly related to importing.

[Bilge]

Move the the iterator wrapping code to a new private method and give it a suitable self-documenting name thus eliminating the need for any comments.

[coveralls]

Coverage decreased (-0.2%) to 98.152% when pulling 096792a on a-barzanti:countable-iterator into c7c6d8a on ScriptFUSION:master.

[coveralls]

Coverage increased (+0.008%) to 98.383% when pulling ac77bf0 on a-barzanti:countable-iterator into c7c6d8a on ScriptFUSION:master.

[Bilge]

Thanks for your changes. The code looks perfect, there are just a few test cases to tidy up.

[Bilge]

There is no need to create a new Porter instance for this test. It should be sufficient to simply reprogram the mock provider's fetch() method to return a different value as shown in testImportOneOfNone() and the tests that follow it.

[Bilge]

During setup() we specify the return value is a default so it may be overridden in tests. Since this is the override we are writing it does not serve any purpose to declare this is to be default also. This byDefault() declaration can simply be removed.

[Bilge]

Prefer specifying an anonymous callback instead of writing a private test method because test cases should be self-contained. This can be expressed using andReturnUsing() as in the following example.

->andReturnUsing(function () {
    yield 'foo';
})

[a-barzanti]

I did the required change and wrote the additional tests.
I agree with you that returning CountableProviderRecords is not much effort, I was just expecting the auto wrap to detect the countable and it just confused me a little the fact that it did not. Regarding the use case, I used Porter to import the Restaurant data form a CSV to take advantage of the functionality you built for the other providers. The CSV Connector was pretty straight forward, I just struggled a little to have the total count right.

[Bilge]

I don't think a "CSV connector" makes much sense because CSV is a file format, not a network transport. Formatting is currently the responsibility of the resource, not the connector. However, I wish there was better separation between data fetching and formatting, but I don't have any good ideas about how to implement that at present.

[a-barzanti]

I think it's a matter of names, if I got it right the connector responsibility is to fetch data from a (network) data source, if you consider the CSV as a data source fetching the data from the file should be a "connector" task.

[Bilge]

It is not simply a matter of naming because CSV is still a file format and not a data source. In this case, the data source is your file system. It might be valid to have a file system connector but such a connector would serve files containing arbitrary data and not be tailored to a specific file type.

[a-barzanti]

I see and use that connector to create a CsvResource that does the actual parsing. If I make the modification to the connector and the resource can I add it to the repo(s).

[Bilge]

I see, in that case perhaps it is just a naming problem, and I agree there could be value in adding such a file system connector to Porter. However, I am more interesting in trying to solve the formatting problem first; that is, placing formatting in a separate class so we would have CsvFormatter instead of CsvResource. I have created issue #4 to track this idea if you want to continue this discussion there.

[coveralls]

Coverage increased (+0.008%) to 98.383% when pulling 40b8599 on a-barzanti:countable-iterator into c7c6d8a on ScriptFUSION:master.

[coveralls]

Coverage increased (+0.008%) to 98.383% when pulling 40b8599 on a-barzanti:countable-iterator into c7c6d8a on ScriptFUSION:master.

[Bilge]

Thank you for your excellent work @a-barzanti.