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

Remove exception from wordsection #99

Merged
merged 4 commits into from
Jan 14, 2023
Merged

Conversation

crjc
Copy link
Contributor

@crjc crjc commented Jan 13, 2023

I wasn't able to use this lib due to this exception being thrown. I've noticed a similar exception in the code base was commented out, so my quick 'fix' was to do the same here.

@PrzemyslawKlys
Copy link
Member

PrzemyslawKlys commented Jan 13, 2023

Well I would prefer you check what type it is missing so maybe we can add a support for it? Just do the debug on it, stop, and see what type it is and add if else. This would allow us to track/and fix it. It's a temporary workaround until we're sure we support all options. the other empty if/else are basically tracked elsewhere so they don't need special treatment. I am afraid by ignoring it you may be loosing some functionality.

I should clean it up, but this is basically still very alpha product.

@crjc
Copy link
Contributor Author

crjc commented Jan 13, 2023

I've added the two types that I found were missing based on my document files. The exception might trip up on other people's documents, but I added it back anyhow.

@PrzemyslawKlys
Copy link
Member

Maybe it would be better to comment it out and just add some sort of output, to log a ticket? on the other hand if it's not defined like you just did people will keep on creating ticket for types - that are not defined. I am not sure what's better.

@crjc
Copy link
Contributor Author

crjc commented Jan 13, 2023

Not sure if this is OK, but I've added a flag to WordDocument which allows for this exception to be disabled. I would have found this useful for my use case earlier.

For example:

WordDocument.ThrowNotImplementedExceptions = false;
using (WordDocument document = WordDocument.Load(filePath)) {
    ...

The exception thrown by default now has a more informative message, summarising what type hasn't been handled and encouraging the user to create an issue here.

@PrzemyslawKlys
Copy link
Member

I've been thinking on it and while I believe your solution would make most sense, I am pro removing all elseif/else and just leave Header/Footer references.

If you won't interact with GutterOnRight or Bidi it will not be removed hence there's no need to throw on it at all. Anything else such as Guitter, PageMargins and so on are already added.

If someone (you?) wants support for GutterOnRight, GutterOnTop or similar

We should simply create new issue and open "support" for given functionality.

It should not throw on this because it's confusing and just not useful. Can you please remove it? I think it will make most sense? What do you think?

And if we really want to support your document type fully we should just understand what those settings are and define them, and fix them in some near future.

@PrzemyslawKlys
Copy link
Member

I've opened the issues:

To track those unsupported elements

@crjc
Copy link
Contributor Author

crjc commented Jan 14, 2023

Thanks! I think that makes sense, so do you think I should remove the two if/else I added, and replace the exception with a simple Debug.WriteLine message?

@PrzemyslawKlys
Copy link
Member

Ye, adding debug.writeline makes sense, so that this can be "seen" but at some point when the project will mature enough we will completely remove it.

@crjc
Copy link
Contributor Author

crjc commented Jan 14, 2023

Here we are, by the way - if this PR looks okay now, are you planning on doing a new release sometime soon?

@PrzemyslawKlys PrzemyslawKlys merged commit e81f390 into EvotecIT:master Jan 14, 2023
@PrzemyslawKlys PrzemyslawKlys changed the title remote exception from wordsection Remove exception from wordsection Jan 14, 2023
@PrzemyslawKlys
Copy link
Member

I will release today after I get back from shops.

@PrzemyslawKlys
Copy link
Member

Scratch that - here you go!

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.

2 participants