Open-XML-SDK icon indicating copy to clipboard operation
Open-XML-SDK copied to clipboard

Published 20 hours ago • verified publisher icon dotnet

Open XML SDK v3: struct IdPartPair

Open sergey-tihon opened this issue 1 year ago • 4 comments

Describe the bug It would be nice to mention all v3-breaking changes in the release notes clearly.

Screenshots Bug with the fix. image

To Reproduce IdPartPait has been converted from class to struct in v3 (most likely for performance reasons).

This change silently broke code that worked fine before and checked the value with null. After the upgrade, the code compile without errors, but some branches became unreachable and app/library no longer works as expected.

Observed behavior No release notes yet ;)

Expected behavior Mention such cases inside the Breaking Changes section in released notes.

Additional context I believe that it will be helpful for apps and library authors who decide to migrate to v3. Found during migrating my fork of PowerTools to SDK v3

sergey-tihon avatar Sep 04 '23 09:09 sergey-tihon

Just found that it was mentioned in the release notes for the beta1. But I personally did not realise from this line how it would affect my code.

image

sergey-tihon avatar Sep 04 '23 14:09 sergey-tihon

Hey @sergey-tihon,

Perhaps you're fine with this now but I thought I'd mention that I see that the compiler does at least inform you that this will always be true:

image

tomjebo avatar Sep 27 '23 16:09 tomjebo

Yes compiler informs with a warning, that it is truly hard to catch If you do not know that something will break.

I think that would be nice to mention in the release notes (breaking changes section) what warnings should be treated as errors during the migration to v3.

sergey-tihon avatar Oct 04 '23 20:10 sergey-tihon

Here's the docs for it: https://github.com/OfficeDev/open-xml-docs/blob/live/docs/migration/migrate-v2-to-v3.md.

I thought I had put a before/after recommendation on how to change this - but apparently it didn't make it there. Happy to take contribution there :)

I think that would be nice to mention in the release notes (breaking changes section) what warnings should be treated as errors during the migration to v3.

My two cents: my default is to have all warnings as error on release builds - if I need to opt out of a warning, I'll do so with a documented reason why so future me (or others) won't need to try to sift through warnings at build to know which ones they should care about.

twsouthwick avatar Dec 05 '23 18:12 twsouthwick