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

Runtime90357 6012 #6885

Merged
merged 7 commits into from
Aug 25, 2023
Merged

Runtime90357 6012 #6885

merged 7 commits into from
Aug 25, 2023

Conversation

manfred-brands
Copy link
Contributor

@manfred-brands manfred-brands commented Aug 23, 2023

Fixes #6012
See also dotnet/runtime#90357

Prevents IndexOutOfRangeException, but also checks formats of non-formatting methods.

@codecov
Copy link

codecov bot commented Aug 23, 2023

Codecov Report

Merging #6885 (046190a) into main (7e4877c) will increase coverage by 0.00%.
Report is 1 commits behind head on main.
The diff coverage is 100.00%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6885   +/-   ##
=======================================
  Coverage   96.39%   96.39%           
=======================================
  Files        1403     1403           
  Lines      330977   331069   +92     
  Branches    10890    10894    +4     
=======================================
+ Hits       319056   319150   +94     
+ Misses       9187     9186    -1     
+ Partials     2734     2733    -1     

@tarekgh
Copy link
Member

tarekgh commented Aug 23, 2023

Fixes dotnet/runtime#90357

Please don't resolve this issue if we merged this PR. We need to have some changes in the runtime repo to remove suppressing AD0001 and ensure porting the fixes to the 8.0 release branch.

CC @buyaa-n

@manfred-brands
Copy link
Contributor Author

Fixes dotnet/runtime#90357

Please don't resolve this issue if we merged this PR. We need to have some changes in the runtime repo to remove suppressing AD0001 and ensure porting the fixes to the 8.0 release branch.

CC @buyaa-n

Updated comment to not fix that issue

@manfred-brands
Copy link
Contributor Author

Not sure why the builds fail. It has probably to do with the new description added.

@buyaa-n
Copy link
Member

buyaa-n commented Aug 24, 2023

Not sure why the builds fail. It has probably to do with the new description added.

Looks it needs to run msbuild /t:pack and push updates

Copy link
Member

@jeffhandley jeffhandley left a comment

Choose a reason for hiding this comment

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

Once CI is fully green and @buyaa-n has signed off on the PR, I approve of this for RC2.

@carlossanlop, when it reaches that point, can you please shepherd this through Tactics and into the RC2 builds?

@carlossanlop
Copy link
Member

Yes, no problem. Assuming this PR is done before Sept 18th (RC2 snap day), your approval @jeffhandley is sufficient (no Tactics involvement needed).

Note: the /backport to <branch> bot does not work in this repo unfortunately, so the backport needs to be created manually. @manfred-brands you'll need to cherry-pick these changes into release/8.0.1xx and create a new PR (do that after merging this PR).

@manfred-brands
Copy link
Contributor Author

Not sure why the builds fail. It has probably to do with the new description added.

Looks it needs to run msbuild /t:pack and push updates

@buyaa-n That did seem to do the trick, but the only change is removing a line for another analyzer from RulesMissingDocumentation.md, which seems completely unrelated to this PR.

Copy link
Member

@buyaa-n buyaa-n left a comment

Choose a reason for hiding this comment

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

Than you @manfred-brands LGTM

@buyaa-n
Copy link
Member

buyaa-n commented Aug 25, 2023

Note: the /backport to bot does not work in this repo unfortunately, so the backport needs to be created manually. @manfred-brands you'll need to cherry-pick these changes into release/8.0.1xx and create a new PR (do that after merging this PR).

@carlossanlop for analyzers repo we used to merge PRs into release branch first then it flows automatically to main, is that same this year or now it's changed? CC @mavasani

@buyaa-n buyaa-n merged commit 972fa0a into dotnet:main Aug 25, 2023
11 checks passed
@carlossanlop
Copy link
Member

carlossanlop commented Aug 25, 2023

@buyaa-n @mavasani since this is the only change we intend to backport to release/8.0.1xx (so far), I propose we do the same as in dotnet/runtime: Merge to main first, then backport to the servicing branch.

@buyaa-n
Copy link
Member

buyaa-n commented Aug 25, 2023

Merged, @manfred-brands we do squash merge so if you planning to put up a port to 8.0 please cherry pick squashed commit.

Or I can do that later

@buyaa-n
Copy link
Member

buyaa-n commented Aug 25, 2023

/backport to release/8.0.1xx

@github-actions
Copy link
Contributor

Started backporting to release/8.0.1xx: https://github.com/dotnet/roslyn-analyzers/actions/runs/5980358523

@buyaa-n
Copy link
Member

buyaa-n commented Aug 25, 2023

@manfred-brands we do squash merge so if you planning to put up a port to 8.0 please cherry pick squashed commit.

Never mind, thanks to #6892 now we have a bot for backporting.

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.

CA2241 should respect new StringSyntaxAttribute.CompositeFormat
5 participants