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

Reorganized services so you can load and write the same file #49

Merged
merged 1 commit into from
Dec 5, 2013

Conversation

gnat42
Copy link
Contributor

@gnat42 gnat42 commented Sep 25, 2013

The previous services were nicely decoupled however because they were
writers were created with blank excel objects, not the object in the
main service being utilized. This way provides a service that can load
files modify them and then output them in multiple formats

This fixes both open issues you currently have #43 and #36

The real issue was that your original services uses the factory method and passes it the 'service' of a blank php excel object.

The previous services were nicely decoupled however because they were
writers were created with blank excel objects, not the object in the
main service being utilized. This way provides a service that can load
files modify them and then output them in multiple formats
@liuggio
Copy link
Owner

liuggio commented Sep 25, 2013

great! Is introducing a sort of BC?
Could you modify the documentation as well?

@gnat42
Copy link
Contributor Author

gnat42 commented Sep 26, 2013

Well it basically removes all of your original services, so anything using the factories or load services are gone. Everything goes through the one service class now. Its somewhat less 'configurable' but allows you to do more. Which is why it replaces your ExcelContainer class with a new class, even though much of the 'API' would be very similar.

I also modified the documentation to match how it should be used. Because this is such a break I wasn't sure how you should merge it. It may be good to be its own newer version so that a composer update doesn't break anyone's code. I'm not really sure the best way. Merging this could potentially break people's projects for sure.

@hectorh30
Copy link

Works to edit an Excel file 👍

liuggio added a commit that referenced this pull request Dec 5, 2013
Reorganized services so you can load and write the same file
@liuggio liuggio merged commit fc6dd31 into liuggio:master Dec 5, 2013
@liuggio
Copy link
Owner

liuggio commented Dec 5, 2013

Version v2 has started :)

@liuggio
Copy link
Owner

liuggio commented Dec 5, 2013

thanks @gnat42

@liuggio
Copy link
Owner

liuggio commented Dec 6, 2013

@gnat42 I reverted your PR, please have a look to the new Factory, WDYT?

@gnat42
Copy link
Contributor Author

gnat42 commented Dec 9, 2013

The factory looks great, only feedback I'd have is to check the 2nd parameter to createWriter.

@liuggio
Copy link
Owner

liuggio commented Dec 9, 2013

good idea the only thing is that the variable in Excel repo is private and hardcoding and duplicating the array is not a great option. Could be great to ask in the repo if they'd like to public the list of the types.

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.

None yet

3 participants