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

Disallow table with null .DataRange when saving #926

Merged

Conversation

igitur
Copy link
Member

@igitur igitur commented Jun 27, 2018

Depends on #908

Throws exception when trying to save a file with a table with no datarange.

@igitur igitur force-pushed the dont-allow-table-with-empty-datarange branch from 306558e to f90e52e Compare June 27, 2018 13:33
@igitur igitur requested a review from Pankraty June 27, 2018 13:33
@igitur igitur added this to the v0.94 milestone Jun 27, 2018
@igitur igitur added the enhancement Feature already exists, but should be enahanced. label Jun 27, 2018
@igitur igitur changed the title Dont allow table with empty datarange Disallow table with null .DataRange when saving Jun 27, 2018

namespace ClosedXML.Excel.Exceptions
{
public class EmptyTableException : Exception
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about inheriting all ClosedXML exceptions from a single one (e.g. abstract ClosedXmlException)? This would make them easier to catch.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, definitely.

@igitur igitur force-pushed the dont-allow-table-with-empty-datarange branch 2 times, most recently from ab5f76f to be7317a Compare June 28, 2018 11:48
public abstract class ClosedXMLException : Exception
{
protected ClosedXMLException()
: base()
Copy link
Member

Choose a reason for hiding this comment

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

Improper indentation


public IXLRange AppendData(IEnumerable data, bool transpose)
{
if (data == null || data is String)
Copy link
Member

Choose a reason for hiding this comment

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

There should also be a check for the empty collection, I suppose.

Copy link
Member

Choose a reason for hiding this comment

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

...and a dedicated test too.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, will address in #908


public IXLRange ReplaceData(IEnumerable data, bool transpose)
{
if (data == null || data is String)
Copy link
Member

Choose a reason for hiding this comment

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

Same here - check if the collection is empty and add a test for it.

@igitur igitur force-pushed the dont-allow-table-with-empty-datarange branch from be7317a to d2b72b6 Compare July 5, 2018 21:38
Copy link
Member

@Pankraty Pankraty left a comment

Choose a reason for hiding this comment

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

I assume this will be rebased after #908 is merged.

@igitur
Copy link
Member Author

igitur commented Jul 7, 2018

Yes, will rebase this. Whenever I say "depends on", that is my intention.

@igitur igitur force-pushed the dont-allow-table-with-empty-datarange branch from d2b72b6 to 550b6af Compare July 8, 2018 11:17
@igitur igitur merged commit 8b93765 into ClosedXML:develop Jul 8, 2018
@igitur igitur deleted the dont-allow-table-with-empty-datarange branch July 8, 2018 11:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature already exists, but should be enahanced.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants