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

Openxmlreader ignorews option #857

Merged
merged 5 commits into from
Jan 22, 2021

Conversation

lklein53
Copy link
Contributor

@lklein53 lklein53 commented Jan 19, 2021

Changes discussed in #848 @twsouthwick

@ghost
Copy link

ghost commented Jan 19, 2021

CLA assistant check
All CLA requirements met.

@lklein53 lklein53 force-pushed the openxmlreader-ignorews-option branch from a25e9e7 to 51296aa Compare January 19, 2021 11:25
@lklein53 lklein53 force-pushed the openxmlreader-ignorews-option branch from 51296aa to c0ba6c5 Compare January 19, 2021 12:03
@lklein53 lklein53 changed the title Openxmlreader ignorews option #848 Openxmlreader ignorews option Jan 19, 2021
Copy link
Member

@twsouthwick twsouthwick left a comment

Choose a reason for hiding this comment

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

This is a great start. Thanks for the fix and the test. I left a few comments.

src/DocumentFormat.OpenXml/OpenXmlPartReader.cs Outdated Show resolved Hide resolved
src/DocumentFormat.OpenXml/OpenXmlPartReader.cs Outdated Show resolved Hide resolved
Lars Klein added 2 commits January 20, 2021 08:08
Introduce new constructor
Restore old constructors
Copy link
Member

@twsouthwick twsouthwick left a comment

Choose a reason for hiding this comment

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

Almost there. A couple of doc issues and a question about old behavior.

src/DocumentFormat.OpenXml/OpenXmlPartReader.cs Outdated Show resolved Hide resolved
src/DocumentFormat.OpenXml/OpenXmlPartReader.cs Outdated Show resolved Hide resolved
src/DocumentFormat.OpenXml/OpenXmlPartReader.cs Outdated Show resolved Hide resolved
Adjust comment
Restore previous whitespace handling
@lklein53
Copy link
Contributor Author

Almost there. A couple of doc issues and a question about old behavior.

Thanks for the review. Sorry messed that up. Wasn't careful enough doing the changes

Copy link
Member

@twsouthwick twsouthwick left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the fix!

@twsouthwick twsouthwick merged commit 242384e into dotnet:master Jan 22, 2021
@lklein53 lklein53 deleted the openxmlreader-ignorews-option branch January 27, 2021 10:59
@lklein53
Copy link
Contributor Author

LGTM. Thanks for the fix!

Hi, sadly today I discovered an issue with the changes. In the case where an xml file has whitespace after the actual content for example:

<document>
  <body>
    ...
  </body>
</document> (space after the >)

the OpenXmlPartReader will crash with an InvalidOperationException as it is trying to get information for a child element with an empty stack

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

2 participants