Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add a .NET 8 target to RequiredTargetFrameworks #46637
base: main
Are you sure you want to change the base?
Add a .NET 8 target to RequiredTargetFrameworks #46637
Changes from 13 commits
d580f5e
1783cbb
a5603a2
70be1f5
5e5a77d
40d8b7a
2360b14
26d0a84
ed1fc69
58578aa
25e4c63
360f83a
cfdd05a
31267bb
92fd10d
55ac13b
1633a89
754c384
d4b6232
0915eec
1419c0e
6ac9055
45bccb4
ca726a9
6d31953
70ce543
ce4e25a
e736815
dc99086
1f1178e
f9bd411
caf33b6
0a8b2dd
f092c0d
daf5b7a
6381bfc
1b1fc6c
0dc4a5b
f1289c6
1dbeb1d
03533bc
83b7539
ca4062b
316a658
6e1bc99
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was producing the following:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this change the behavior of the method? Do we know what the intended behavior is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an error not a warning, so the current code cannot stay as-is. Someone from the communication team should take a look to make sure this is the intended behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an obvious mistake, as the parameter has not been initialized at this point. My suspicion is that the conditional never/rarely matches so this code never actually executes in any kind of test.
Maddy's fix looks to be the obvious one to correct the behavior based on the method name. We removed
matchingKvp.Key
and we want to return it.Let's reach out to the ACS team and ask them to sign-off. If they don't respond in a week, here's how I'd like to proceed with this:
That will block CI for any future changes/releases.
Thing we need to check first: can this library ever get randomly pulled into the Core test suite? If so, we need to make sure that doesn't happen since we're adding a consistent failure.