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

Adjust embeddedresource culture warning #11298

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

JanKrivanek
Copy link
Member

@JanKrivanek JanKrivanek commented Jan 17, 2025

Fixes #11313

Context

The newly added MSB3002 has a breaking potential (while it's very correct). So making it an opt-in behavior

@JanKrivanek JanKrivanek marked this pull request as ready for review January 20, 2025 09:12
@SimaTian
Copy link
Member

What is our heuristic for selecting between

  • changewave
  • opt-in
  • opt-out (different than a changewave, do we ever do that?)
    please?

@maridematte
Copy link
Contributor

Isn't this the exact case why we have changewaves? It is a good change that we do want to enforce, but with an adoption period for breaking cases / edge cases.

@JanKrivanek
Copy link
Member Author

We discussed this offline with @rainersigwald and decided for opt-in behavior. So ChangeWave (which was here previously) is not an optimal choice now.

The opt-in/out decision is basicaly per case decision based on the "breaking potential" vs "improvement"

Copy link
Member

@JanProvaznik JanProvaznik left a comment

Choose a reason for hiding this comment

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

:shipit:

@rainersigwald
Copy link
Member

changewaves are "we believe no one will be impacted by this but we aren't totally sure so let's evaluate". That's not appropriate here: before release we've broken the two repos closest to us, so we know it's breaky.

Opt-ins are for behavior that is like this: breaky but also "clearly right". Then we can have the SDK opt in in new .NET versions or apply BuildChecks to encourage people to opt into stuff.

Opt-outs without changewaves used to be how almost everything was done (which is why we have so many) but now we prefer changewaves (which go away eventually, leaving less ugly code). However, if we expect people to reasonably want differing behavior in different circumstances they can be reasonable.

Copy link
Member

@rainersigwald rainersigwald left a comment

Choose a reason for hiding this comment

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

This targets main. Doesn't it need to be a 17.13 QB fix?

@JanKrivanek JanKrivanek changed the base branch from main to vs17.13 January 21, 2025 17:34
@JanKrivanek JanKrivanek requested a review from a team as a code owner January 21, 2025 17:34
@JanKrivanek JanKrivanek changed the base branch from vs17.13 to main January 21, 2025 17:35
@JanKrivanek JanKrivanek marked this pull request as draft January 21, 2025 17:40
@JanKrivanek
Copy link
Member Author

Ported to #11320
Let's then get it by codeflow

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

Successfully merging this pull request may close these issues.

EmbeddedResource - prevent breaking change
6 participants