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

Open XML SDK v3: struct IdPartPair #1520

Open
sergey-tihon opened this issue Sep 4, 2023 · 4 comments
Open

Open XML SDK v3: struct IdPartPair #1520

sergey-tihon opened this issue Sep 4, 2023 · 4 comments

Comments

@sergey-tihon
Copy link

sergey-tihon commented Sep 4, 2023

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
Copy link
Author

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

@tomjebo tomjebo self-assigned this Sep 27, 2023
@tomjebo
Copy link
Collaborator

tomjebo commented Sep 27, 2023

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 tomjebo closed this as completed Sep 27, 2023
@sergey-tihon
Copy link
Author

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.

@twsouthwick
Copy link
Member

twsouthwick commented Dec 5, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants