Skip to content

Hook up doc links to IDExxxx rules#48471

Merged
mavasani merged 7 commits intodotnet:masterfrom
mavasani:IdeHelpLinks
Oct 12, 2020
Merged

Hook up doc links to IDExxxx rules#48471
mavasani merged 7 commits intodotnet:masterfrom
mavasani:IdeHelpLinks

Conversation

@mavasani
Copy link
Contributor

@mavasani mavasani commented Oct 9, 2020

Fixes #41324

I have also done some additional refactoring to combine AbstractBuiltInCodeStyleDiagnosticAnalyzer and AbstractCodeStyleDiagnosticAnalyzer into a single type.

Fixes dotnet#41324
I have also done some additional refactoring to combine `AbstractBuiltInCodeStyleDiagnosticAnalyzer` and `AbstractCodeStyleDiagnosticAnalyzer` into a single type.
DiagnosticSeverity.Hidden,
isEnabledByDefault: true,
description: description,
helpLinkUri: $"https://docs.microsoft.com/dotnet/fundamentals/code-analysis/style-rules/{id}",
Copy link
Contributor Author

@mavasani mavasani Oct 9, 2020

Choose a reason for hiding this comment

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

@Youssef1313
Copy link
Member

Do we now need some way to track missing documentation like roslyn-analyzers repo?

DiagnosticSeverity.Hidden,
isEnabledByDefault: true,
description: description,
helpLinkUri: $"https://docs.microsoft.com/dotnet/fundamentals/code-analysis/style-rules/{id}",
Copy link
Member

@Youssef1313 Youssef1313 Oct 9, 2020

Choose a reason for hiding this comment

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

What about the rules that are documented together?
The link for them takes the form of https://docs.microsoft.com/dotnet/fundamentals/code-analysis/style-rules/idexxxx-ideyyyy

I'd use the F1 link here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about the rules that are documented together?

We have redirects for these, I verified this will work after my latest open PR is merged.

I'd use the F1 link here.

We considered it, but it does not work for all rule IDs. Also confirmed with @gewarren that we should not rely on msdn redirections for help links.

For context, if I type out https://msdn.microsoft.com/query/roslyn.query?appId=roslyn&k=k(CA2000) it seems to redirect properly. Doesn't seem to work for all IDs though, for example https://msdn.microsoft.com/query/roslyn.query?appId=roslyn&k=k(CA1000) took me to the index (not sure if that is only for the first rule though). It doesn't work even for some rule in middle https://msdn.microsoft.com/query/roslyn.query?appId=roslyn&k=k(CA2234).

Copy link
Member

Choose a reason for hiding this comment

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

We have redirects for these, I verified this will work after my latest open PR is merged.

Great. There is no problem then if the redirections for these will always be maintained for future rules. 🎉

For context, if I type out https://msdn.microsoft.com/query/roslyn.query?appId=roslyn&k=k(CA2000) it seems to redirect properly. Doesn't seem to work for all IDs though, for example https://msdn.microsoft.com/query/roslyn.query?appId=roslyn&k=k(CA1000) took me to the index (not sure if that is only for the first rule though). It doesn't work even for some rule in middle https://msdn.microsoft.com/query/roslyn.query?appId=roslyn&k=k(CA2234).

Probably the F1 service redirections aren't working because these f1_keywords existed in VS docs. The service seems to have some cache of where each keyword exists. So it should work properly after some time. (This isn't to say we should rely on F1 service, just putting my thought on why it might be not working properly)

@mavasani
Copy link
Contributor Author

mavasani commented Oct 9, 2020

Do we now need some way to track missing documentation like roslyn-analyzers repo?

Possibly, but not in the scope of this PR.

Comment on lines +836 to +837
# IDE0047, ArithmeticBinaryParentheses
dotnet_style_parentheses_in_arithmetic_binary_operators = always_for_clarity:silent
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this the expected behavior even before this PR? Was this a bug somewhere that got fixed with this PR? or is there something I'm not seeing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the analyzer was not registering its supported options in its ctor, hence configuration code fixes were not working for it.

@mavasani mavasani marked this pull request as ready for review October 9, 2020 20:05
@mavasani mavasani requested a review from a team as a code owner October 9, 2020 20:05
Copy link
Contributor

@jmarolf jmarolf left a comment

Choose a reason for hiding this comment

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

love to see it

@ghost
Copy link

ghost commented Oct 12, 2020

Hello @mavasani!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Auto-approval

@mavasani
Copy link
Contributor Author

Thank you @jmarolf @JoeRobich!

@mavasani mavasani merged commit 346de17 into dotnet:master Oct 12, 2020
@ghost ghost added this to the Next milestone Oct 12, 2020
@mavasani mavasani deleted the IdeHelpLinks branch October 12, 2020 21:42
@Cosifne Cosifne modified the milestones: Next, 16.9.P1 Oct 12, 2020
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.

Better help link for IDE analyzers

5 participants