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

Check XML doc parameter names for FSharp.Core via opt-in checking #10118

Merged
merged 29 commits into from
Sep 15, 2020

Conversation

dsyme
Copy link
Contributor

@dsyme dsyme commented Sep 14, 2020

  1. Fixes mistakes in FSharp.Core XML docs

  2. Fix mistakes in FCS docs

  3. Implements the opt-in XML document checking /warnon:3390 and applies it to FSharp.Core,

    When /warnon:3390 is enabled (e.g. for FCS and FSharp.Core):

    • checks <param name="xyz"> and <paramref name="xyz"> it checks that xyz is a valid parameter name for the thing being documents

    • checks that if any parameter is given docs, then all are given docs

  4. Adds the ability to get the elaborated (e.g. with summary tags) XML from the FCS API, which we should always have allowed. This will become more important if/when we support cross-reference cref elaboration in XML docs.

  5. Fixes a bug where you couldn't add documentation to an implicit constructor, see this stack overflow question.

Aside: This PR also contains the beginning of code for cross-reference checking and elaboration, allowing the use of "short name" forms for cross references, including rename-refactor on these. However that code is not enabled.

@dsyme dsyme changed the title Add parameter checking to opt-in XML document checking and use in FSharp.Core Check XML doc parameter names for FSharp.Core via opt-in checking Sep 14, 2020
@dsyme
Copy link
Contributor Author

dsyme commented Sep 14, 2020

This is ready (once green - having some niggling issues)

@dsyme
Copy link
Contributor Author

dsyme commented Sep 15, 2020

This is now ready for review

@dsyme
Copy link
Contributor Author

dsyme commented Sep 15, 2020

@cartermp I think it makes sense to document

  • /warnon:1182 - unused variables
  • /warnon:3390 - check XML docs for validity and completeness

in the Microsoft F# docs. I was going to put something under "docs" but it's likely better in the actual guide

@dsyme
Copy link
Contributor Author

dsyme commented Sep 15, 2020

Side note: I wanted to add separate warnings for

  • give warning if elements do not have documentation

  • give warning if an element doesn't have full parameter information

However there are some issues with that. Notably

  1. It forces people to provide documentation for implicit constructors (or else have a signature file). This is awkward, e.g.

    /// The type docs
    type C
        /// The implicit constructor docs
        (x: int, y: int) = ...
    

    I don't really want a feature that will force this on people. We should probably allow a special section in the docs for the type e.g.

    /// <summary>The type docs</summary>
    /// <ctor><summary>The implicit constructor docs</summary></ctor>
    type C(x: int, y: int) = ...
    
  2. It is also common to have incomplete docs (e,g, no parameter docs) in an implementation but have complete docs in a signature. So enforcement of completeness should only be done on the signature docs if a signature is present.

But those are for later. What we have in this PR is enough to pin down quality for FCS and FSharp.Core docs.

@@ -2,7559 +2,7594 @@
<xliff xmlns="urn:oasis:names:tc:xliff:document:1.2" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" version="1.2" xsi:schemaLocation="urn:oasis:names:tc:xliff:document:1.2 xliff-core-1.2-transitional.xsd">
<file datatype="xml" source-language="en" target-language="ja" original="../FSComp.resx">
<body>
<trans-unit id="chkFeatureNotLanguageSupported">
Copy link
Contributor

Choose a reason for hiding this comment

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

How did this enormous diff occur?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure

@KevinRansom KevinRansom merged commit 20dec2a into dotnet:main Sep 15, 2020
@dsyme
Copy link
Contributor Author

dsyme commented Sep 15, 2020

@KevinRansom Thank you, it's great to have this in

@dsyme
Copy link
Contributor Author

dsyme commented Sep 15, 2020

Relevant documentation PR: dotnet/docs#20645

nosami pushed a commit to xamarin/visualfsharp that referenced this pull request Feb 23, 2021
…tnet#10118)

* optionally check xml doc comments for xml validity

* check xml docs

* check xml docs

* xml checks 2

* fix baselines

* check parameter names

* fix baseline

* check parameters names

* allow delegate param doc

* fix FSHarp.Core doc problems

* fix docs

* fix docs

* fix diagnostics

* fix diagnostics

* update baselines

* provide elaborated xml docs in FCS API

* fix test

* use 38 ver

* fix build

* check docs of FCS

* start to fix FCS docs

* complete some FCS docs and enable doc checking

* fix duplicate checking

* fix duplicate checking

* update baselines and xlf

* correct xml text for legacy test

Co-authored-by: Don Syme <[email protected]>
nosami pushed a commit to xamarin/visualfsharp that referenced this pull request Jan 26, 2022
…tnet#10118)

* optionally check xml doc comments for xml validity

* check xml docs

* check xml docs

* xml checks 2

* fix baselines

* check parameter names

* fix baseline

* check parameters names

* allow delegate param doc

* fix FSHarp.Core doc problems

* fix docs

* fix docs

* fix diagnostics

* fix diagnostics

* update baselines

* provide elaborated xml docs in FCS API

* fix test

* use 38 ver

* fix build

* check docs of FCS

* start to fix FCS docs

* complete some FCS docs and enable doc checking

* fix duplicate checking

* fix duplicate checking

* update baselines and xlf

* correct xml text for legacy test

Co-authored-by: Don Syme <[email protected]>
nosami pushed a commit to xamarin/visualfsharp that referenced this pull request Jan 26, 2022
…tnet#10118)

* optionally check xml doc comments for xml validity

* check xml docs

* check xml docs

* xml checks 2

* fix baselines

* check parameter names

* fix baseline

* check parameters names

* allow delegate param doc

* fix FSHarp.Core doc problems

* fix docs

* fix docs

* fix diagnostics

* fix diagnostics

* update baselines

* provide elaborated xml docs in FCS API

* fix test

* use 38 ver

* fix build

* check docs of FCS

* start to fix FCS docs

* complete some FCS docs and enable doc checking

* fix duplicate checking

* fix duplicate checking

* update baselines and xlf

* correct xml text for legacy test

Co-authored-by: Don Syme <[email protected]>
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.

3 participants