Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Raise the default warning level in tests#47077

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
RikkiGibson merged 43 commits intodotnet:masterfromYoussef1313:patch-19
Oct 17, 2020

Conversation

@Youssef1313
Copy link
Member

@Youssef1313Youssef1313 commentedAug 23, 2020•
edited
Loading

Fixes#46976

@RikkiGibson Could you have a look if what I'm doing until now is the correct thing?

@Youssef1313Youssef1313 requested a review froma team as acode ownerAugust 23, 2020 11:58
@Youssef1313Youssef1313 marked this pull request as draftAugust 23, 2020 12:01
@jcouvjcouv added Area-Compilers CommunityThe pull request was submitted by a contributor who is not a Microsoft employee. labelsAug 23, 2020

publicclassAnalyzerFileReferenceAppDomainTests:TestBase
{
privateconstintMaxWarningLevel=9999;
Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Because this project doesn't have a reference toTestOptions, I created this const. But I don't feel this approach is good. This const is defined in a few other tests in this project.

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps it would be reasonable to define it next to

internalconstintDefaultWarningLevel=4;

Copy link
Contributor

Choose a reason for hiding this comment

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

Why isn't max warning level justint.MaxValue ?

Copy link
MemberAuthor

@Youssef1313Youssef1313Aug 24, 2020•
edited
Loading

Choose a reason for hiding this comment

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

It should work, but it was documentedhere as 9999 so I matched that.

@Youssef1313
Copy link
MemberAuthor

@jaredpar@RikkiGibson I've addressed your feedback and also updated more tests (there are still a few other tests that needs to be updated too).

@Youssef1313Youssef1313 marked this pull request as ready for reviewSeptember 21, 2020 16:49
@Youssef1313
Copy link
MemberAuthor

Pinging@RikkiGibson
If this is still needed, can you have a look at#47077 (comment)

@RikkiGibson
Copy link
Member

#47077 (comment)

I think we should include the new warning inCS0722ERR_ReturnTypeIsStaticClass01--however I don't have a strong opinion about this.

@Youssef1313
Copy link
MemberAuthor

@RikkiGibson I included it. Can you re-run the failed jobs? the current failures seem unrelated to the changes.

If this is ready to go, you may want consider a squash/merge. The number of commits here is unnecessarily large

@Youssef1313
Copy link
MemberAuthor

@RikkiGibson Build is green now.

RikkiGibson reacted with heart emoji

@RikkiGibsonRikkiGibson self-assigned thisOct 5, 2020
@RikkiGibson
Copy link
Member

@dotnet/roslyn-compiler for another review. It's a test only change, but sweeping enough that it feels like getting another pair of eyes on it would be good.

Suggest starting with the following files:

Then skimming the rest of it.

@jcouv
Copy link
Member

Looking

@jcouvjcouv self-assigned thisOct 16, 2020
@Youssef1313
Copy link
MemberAuthor

Note: commit-by-commit review isnot recommended at all, the commits here are messy. I'd also recommend a squash/merge to not pollute the master history.

/// <param name="optimizationLevel">The optimization level of the created compilation options.</param>
/// <param name="allowUnsafe">A boolean specifying whether to allow unsafe code. Defaults to false.</param>
/// <returns>A CSharpCompilationOptions with the specified <paramref name="outputKind"/>, <paramref name="optimizationLevel"/>, and <paramref name="allowUnsafe"/>.</returns>
internalstaticCSharpCompilationOptionsCreateTestOptions(OutputKindoutputKind,OptimizationLeveloptimizationLevel,boolallowUnsafe=false)
Copy link
Member

@jcouvjcouvOct 16, 2020•
edited
Loading

Choose a reason for hiding this comment

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

internal [](start = 8, length = 8)

private? Never mind. Somehow did a bad search ;-) #Closed

Copy link
Member

@jcouvjcouv left a comment

Choose a reason for hiding this comment

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

LGTM Thanks (iteration 43). I'll re-run CI to make sure we didn't introduce new conflicts this the PR was last run.

@Youssef1313
Copy link
MemberAuthor

@jcouv That was already done. The current CI run was succeeded 2 hours ago in 1h 44m 11s

@RikkiGibsonRikkiGibson merged commitb43d165 intodotnet:masterOct 17, 2020
@ghostghost added this to theNext milestoneOct 17, 2020
@RikkiGibson
Copy link
Member

Thanks for the contribution@Youssef1313!

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@jaredparjaredparjaredpar left review comments

@RikkiGibsonRikkiGibsonRikkiGibson approved these changes

@jcouvjcouvjcouv approved these changes

+1 more reviewer

@ryzngardryzngardryzngard left review comments

Reviewers whose approvals may not affect merge requirements

Labels

Area-CompilersCommunityThe pull request was submitted by a contributor who is not a Microsoft employee.

Projects

None yet

Milestone

16.9.P2

Development

Successfully merging this pull request may close these issues.

Raise the default WarningLevel to latest in tests

6 participants

@Youssef1313@RikkiGibson@jcouv@jaredpar@ryzngard@allisonchou
[8]ページ先頭
©2009-2025 Movatter.jp