Skip to content
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

One to many reader #78

Merged
merged 1 commit into from
Jun 2, 2014
Merged

One to many reader #78

merged 1 commit into from
Jun 2, 2014

Conversation

adam-paterson
Copy link
Contributor

We required the ability to add two readers into the same workflow. We have Order and OrderLineData in two CSVs. This reader allows you to this by matching the join fields of two readers.

The test class shows perfectly what we're trying to achieve here.

@AydinHassan
Copy link
Contributor

This also has a pretty cool side effect. You could potentially pass the OneToMany reader with another reader into a new OneToMany reader to merge potentially infinite amount of reader sources. If it's something useful for the project, we'll write up some docs for it.

@adam-paterson
Copy link
Contributor Author

Side affect? ;) it was intensional right

@Baachi
Copy link
Collaborator

Baachi commented May 23, 2014

Looks good :)
Can you add a note in the documentation?

@ddeboer
Copy link
Owner

ddeboer commented May 23, 2014

Interesting idea!

But can't you just do two imports instead, first the orders and then the line items? Each of those imports only needs one reader.

@adam-paterson
Copy link
Contributor Author

This would be used in cases when two imports wouldn't suffice. For example, a writer expecting a particular set of data from two sources.

One of the models we use to save the data requires the items to be populated on the order before it can be saved. So we can't do two imports.

@ddeboer
Copy link
Owner

ddeboer commented May 23, 2014

Okay, makes sense.


/**
* Class OneToManyReader
* @package Ddeboer\DataImport\Reader
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this line.

@ddeboer
Copy link
Owner

ddeboer commented May 29, 2014

Code is looking good now. Can you add an entry to the readme for this reader?

@AydinHassan
Copy link
Contributor

Sure - will do this tomorrow!

@AydinHassan
Copy link
Contributor

@ddeboer All good?

The resulting data will look like:

```
\\Row 1
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be //

@ddeboer
Copy link
Owner

ddeboer commented Jun 1, 2014

Small small issues remaining. If you fix them and squash commits, this is good to be merged.

@AydinHassan
Copy link
Contributor

Done :)

@AydinHassan
Copy link
Contributor

This is failing due to "symfony/console": ">=2.2", in composer.json. Symfony 2.5 was released today and thus the ProgressHelper has been deprecated. You can merge #73 and it will fix this :D

@AydinHassan
Copy link
Contributor

I've just rebased this against master again :)

ddeboer added a commit that referenced this pull request Jun 2, 2014
@ddeboer ddeboer merged commit bbdeb78 into ddeboer:master Jun 2, 2014
@AydinHassan
Copy link
Contributor

Thanks for merging!

@AydinHassan AydinHassan deleted the one-to-many-reader branch June 2, 2014 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants