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

Add <inheritdoc /> in migrations #27268

Merged
merged 1 commit into from
Jan 28, 2022
Merged

Conversation

roji
Copy link
Member

@roji roji commented Jan 24, 2022

And suppress CA1814 if multidimensional seed data is present.

Closes #10695

@roji roji requested a review from a team January 24, 2022 15:20
@roji roji marked this pull request as draft January 24, 2022 15:21
@roji roji marked this pull request as ready for review January 24, 2022 19:40
@roji roji requested a review from dougbu as a code owner January 24, 2022 19:40
@roji roji marked this pull request as draft January 24, 2022 20:01
And suppress CA1814 if multidimensional seed data is present.

Closes dotnet#10695
@roji roji marked this pull request as ready for review January 25, 2022 19:41
@@ -78,6 +78,14 @@ public override string Language
.AppendLine()
.AppendLine("#nullable disable");

// Suppress "Prefer jagged arrays over multidimensional" when we have a seeding operation with a multidimensional array
if (HasMultidimensionalArray(upOperations.Concat(downOperations)))
Copy link
Member

Choose a reason for hiding this comment

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

How is this related to inheritdoc?

Copy link
Member Author

Choose a reason for hiding this comment

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

It isn't - it's another thing in the same area that's tracked by that same issue (see #10695 (comment)). Do you prefer I split it out, or think we shouldn't do it?

Copy link
Member

Choose a reason for hiding this comment

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

I think we should at least discuss. I really hate adding suppressions, especially for overzealous warnings like this. Is this a modern analyzer warning? If so, what is the justification for it? If it's an old FxCop warning then...

Copy link
Member Author

@roji roji Jan 27, 2022

Choose a reason for hiding this comment

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

This rule is reported by the standard .NET analyzers that started shipping as part of the SDK in .NET 5.0 (see docs). It's indeed not enabled by default, but anyone enabling e.g. all the "performance" category diagnostics gets it.

I also hate adding suppressions, which is why I went to the extra trouble of adding it only where we actually generate a multidimensional array. I do think it reduces a bit of needless friction for users who enable most of the rules, but if you're against it we can discuss and remove.

@bricelam bricelam mentioned this pull request Jan 28, 2022
@roji roji merged commit a204b62 into dotnet:main Jan 28, 2022
@roji roji deleted the MigrationsInheritDoc branch January 28, 2022 21:53
@@ -28,6 +28,6 @@
<MicrosoftExtensionsLoggingVersion>7.0.0-alpha.1.22066.4</MicrosoftExtensionsLoggingVersion>
</PropertyGroup>
<PropertyGroup Label="Other dependencies">
<MicrosoftCodeAnalysisVersion>3.7.0</MicrosoftCodeAnalysisVersion>
<MicrosoftCodeAnalysisVersion>4.0.1</MicrosoftCodeAnalysisVersion>
Copy link
Contributor

@bricelam bricelam Jan 28, 2022

Choose a reason for hiding this comment

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

Can you move the following comment to here?

<!-- NB: Version affects the minimum required Visual Studio version -->
<PackageReference Include="Microsoft.CodeAnalysis.CSharp" Version="$(MicrosoftCodeAnalysisVersion)" PrivateAssets="all" />

Copy link
Contributor

@bricelam bricelam Jan 28, 2022

Choose a reason for hiding this comment

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

Doh, I was too slow to comment; you already merged

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh sorry... Can slip it into the next thing (either you or me).

Any idea what 4.0.1 restricts us to in terms of VS?

Copy link
Contributor

Choose a reason for hiding this comment

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

2022 (probably worth putting https://aka.ms/roslyn-packages in the comment)

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

Successfully merging this pull request may close these issues.

Add inheritdoc to migration files
3 participants