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

Do not check XmlDocs unless required #14156

Merged
merged 7 commits into from
Nov 22, 2022
Merged

Conversation

DedSec256
Copy link
Contributor

Currently, XmlDoc check is performed even if the corresponding diagnostic (3390) is not enabled in the .fsproj.

This PR suggests adding an option to DiagnosticOptions to not call analysis if not required.

@DedSec256 DedSec256 changed the title Do not check XmlDocs unless required [WIP] Do not check XmlDocs unless required Oct 21, 2022
Copy link
Member

@T-Gro T-Gro left a comment

Choose a reason for hiding this comment

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

Looks good in general, but it would be good to cover the functionality with tests covering the positive and negative cases.

@DedSec256
Copy link
Contributor Author

@T-Gro, not sure if tests are needed here as this PR does not change the visible behavior in any way. The problem that this PR fixes is that checking XML docs is always done, even if its results are not required and therefore ignored. This PR allows analysis to be performed only when its results are required (i.e. warnon:3390 option is enabled in .fsproj).

@T-Gro
Copy link
Member

T-Gro commented Oct 24, 2022

@T-Gro, not sure if tests are needed here as this PR does not change the visible behavior in any way. The problem that this PR fixes is that checking XML docs is always done, even if its results are not required and therefore ignored. This PR allows analysis to be performed only when its results are required (i.e. warnon:3390 option is enabled in .fsproj).

That is what I meant - if the warnon is enabled, a warning should still be produced.

@DedSec256
Copy link
Contributor Author

There are a lot of tests for XmlDocs checking in
https://github.com/dotnet/fsharp/blob/main/tests/FSharp.Compiler.ComponentTests/Language/XmlComments.fs

with enabled 3390 diagnostic

/// Turns on checks that check integrity of XML doc comments
let withXmlCommentChecking (cUnit: CompilationUnit) : CompilationUnit =
withOptionsHelper [ "--warnon:3390" ] "withXmlCommentChecking is only supported for F#" cUnit

I will add one test that checks for the absence of warnings with disabled diagnostic

T-Gro
T-Gro previously approved these changes Oct 24, 2022
@T-Gro T-Gro requested review from 0101, KevinRansom and abonie November 4, 2022 16:33
abonie
abonie previously approved these changes Nov 14, 2022
@T-Gro T-Gro dismissed stale reviews from abonie and themself via e6fae73 November 22, 2022 10:00
@T-Gro T-Gro requested a review from a team as a code owner November 22, 2022 10:00
@T-Gro T-Gro requested a review from abonie November 22, 2022 10:00
@0101 0101 merged commit 6fa4fd6 into dotnet:main Nov 22, 2022
@DedSec256 DedSec256 deleted the dedsec256-checkXmlDocs branch November 22, 2022 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants