search

salvois / LargeXlsx

A .net library to write large Excel files in XLSX format with low memory consumption using streamed write.
Other
204 stars 34 forks source link

Implement dispose pattern to allow for derived XlsxWriter #11

Closed AntonyCorbett closed 2 years ago

AntonyCorbett commented 2 years ago

Added the protected Dispose method to allow for a derived XlsxWriter to clean up the base XlsxWriter. Also added _disposed flag to make disposal idempotent.

salvois commented 2 years ago

Hi @AntonyCorbett , thanks for your contribution, I'll have a look as soon as possible! Out of curiosity, can you provide an example scenario where you may want to subclass XlsxWriter? Salvo

AntonyCorbett commented 2 years ago

Hi @salvois Thanks for your hard work on this code. - very useful. My use case would be where I wanted a method on the writer, "WriteHyperlink". It's not a very persuasive reason :) but the alternative is to make XlsxWriter sealed which might break existing code for some. Regards Antony

salvois commented 2 years ago

Hi @AntonyCorbett , I see your point and why you may have wanted to subclass XlsxWriter. To be honest, I never thought about XlsxWriter being possibly used as a base class, because my brain usually shies away from inheritance, and I pretend everything is sealed by default (my limitation, I know). I'm afraid allowing subclasses of XlsxWriter may open to unexpected behaviors, so I'm inclined to consider such usage of the class unintended, make it sealed, and treat it not being sealed as a bug. Strictly speaking, as you point out, that would be a breaking change but such usage should have been discouraged in the first place because the dispose pattern was not there. Please feel free to let me know your opinion about this. The idempotence part may be especially important. Also, let me know about your WriteHyperlink method, that could be a nice addition to the library! Thanks, Salvo

AntonyCorbett commented 2 years ago

Hi @salvois

Please feel free to let me know your opinion about this.

Yes, making it sealed seems a good solution.