Skip to content

Add a note explaining the XXXX in the rule number to InternalsVisibleTo page #44240

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

Merged
merged 3 commits into from
Jan 15, 2025

Conversation

BartoszKlonowski
Copy link
Contributor

@BartoszKlonowski BartoszKlonowski commented Jan 13, 2025

This pull request closes #44197.

Initially I aimed to make a note about necessity to replace CAXXXX with the proper CA's number that the page describes, but when writing that I noticed it makes the idea of having the generic page useless if we still need to provide non-generic NOTE for each page. At the same time I see the point of having that include a generic, so I didn't want to remove it totally and break DRY rule.

Eventually I went for extracting only the "non-generic" part, which is the code snippet, to avoid confusions and to still be able to include the part that is repeated for each CA using this IVT variable.


Internal previews

📄 File 🔗 Preview link
docs/fundamentals/code-analysis/quality-rules/ca1812.md CA1812: Avoid uninstantiated internal classes
docs/fundamentals/code-analysis/quality-rules/ca1852.md "CA1852: Seal internal types"

@BartoszKlonowski BartoszKlonowski requested review from gewarren and a team as code owners January 13, 2025 18:25
@dotnetrepoman dotnetrepoman bot added this to the January 2025 milestone Jan 13, 2025
@dotnet-policy-service dotnet-policy-service bot added dotnet-fundamentals/svc community-contribution Indicates PR is created by someone from the .NET community. labels Jan 13, 2025
@gewarren
Copy link
Contributor

@BartoszKlonowski I don't love this fix. For one thing, there is now a hanging colon at the end of the include file, so the include file can't stand "on its own". The second reason is that there are other include files that use this same pattern because they're shared between multiple CA rules, for example:

So we're breaking the pattern if we merge this PR. I think a better fix is to note, in the include file, that the "XXXX" should be replaced with the corresponding rule number.

@BartoszKlonowski
Copy link
Contributor Author

Alright, putting the note in the include file didn't actually cross my mind, but I agree this is the good idea!

@BartoszKlonowski BartoszKlonowski changed the title Extract CAXXXX code snippet from InternalsVisibleTo generic page to avoid confusions Add a note explaining the XXXX in the rule number to InternalsVisibleTo page Jan 15, 2025
@BartoszKlonowski
Copy link
Contributor Author

@gewarren Please let me know how you find the latest changes

Copy link
Contributor

@gewarren gewarren left a comment

Choose a reason for hiding this comment

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

Thank you @BartoszKlonowski. I left a suggestion for slightly more succinct wording. Also, would you be able to add this note to the other shared include files that state CAXXXX? Thank you so much for your help with this.

@gewarren gewarren enabled auto-merge (squash) January 15, 2025 19:50
@gewarren gewarren merged commit 42b412a into dotnet:main Jan 15, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community-contribution Indicates PR is created by someone from the .NET community. dotnet-fundamentals/svc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ignore InternalsVisibleTo attribute section sample does not show the rule ID
3 participants