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

Ods writer: add support for basic text styling #105

Closed
agopaul opened this issue Feb 27, 2017 · 3 comments
Closed

Ods writer: add support for basic text styling #105

agopaul opened this issue Feb 27, 2017 · 3 comments

Comments

@agopaul
Copy link
Contributor

agopaul commented Feb 27, 2017

The Ods writer doesn't support font styles like color, size, bold, italic, single underline, cell background.

Of course, this is a feature rather than a bug, but since I already implemented this on PHPExcel (see PR pending for PHPExcel PHPOffice/PHPExcel#785) I thought maybe I could port the PR here.

Also, since PhpSpreadsheet now has a working testing/CI environment, I'm writing a few unit tests for the Ods writer too.

I will reference the PR to this issue when it's ready

@PowerKiKi
Copy link
Member

Thanks, looking forward to it.

For unit tests, it would be great to limit the use of binary files in the repository, either by not using any at all or re-using the existing one. But I guess you won't need any of them to test writing things.

@agopaul
Copy link
Contributor Author

agopaul commented Feb 28, 2017

I shouldn't need any binary file, but since some methods return XML strings I'm wondering which is the preferred method to test XML output. XML it's not binary per se, but it can increase the weight on the repository in the long term. On the other hand, XML is still plain text and it's easy to merge if that was your point.

I can see two solutions for these kinds of tests where the XML output needs to be tested:

  • take the output and test it against an XML file inside the repository (using PHPUnit's assertXmlStringEqualsXmlFile(), which uses DOMDocument and makes a comparison at structure level, not simple string comparison)
  • take the output and parse it with DOMDocument or SimpleXML, checking each attributes and values

Both solutions have pros and cons. Most notably, the second approach could result in more verbose unit tests (lots of assertions or data inside the tests).
On the other hand, the first approach puts a little more weight on the repository and it could theoretically result in less maintainable (but likely more precise) tests.

My choice between the two would be the first approach because is can be more precise and, of course, the tests are faster to write.

I looked in the test suite for tests which used one solution or the other but I couldn't find any.

What do you think should be the preferred solution? I guess this can become a guideline for future tests too.

@PowerKiKi
Copy link
Member

I am OK with the first solution. Hopefully it does not becomes too huge too soon, but like you said it seems to be the easiest way for now.

PowerKiKi added a commit that referenced this issue Mar 6, 2017
Ods writer: add support for basic text styling (Fix #105)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants