Skip to content

Add docs for ISymbol and its derived interfaces that say that they shoul... - #809

Merged
gafter merged 1 commit into
dotnet:masterfrom
pdelvo:SymbolDoc
Mar 31, 2015
Merged

Add docs for ISymbol and its derived interfaces that say that they shoul...#809
gafter merged 1 commit into
dotnet:masterfrom
pdelvo:SymbolDoc

Conversation

@pdelvo

@pdelvo pdelvo commented Feb 24, 2015

Copy link
Copy Markdown
Contributor

...d not be implemented outside Roslyn.

Fixes #632.

@gafter gafter added Concept-API This issue involves adding, removing, clarification, or modification of an API. Area-Compilers Tenet-Compatibility Violation of forwards/backwards compatibility in a design-time piece. labels Feb 25, 2015
@mattwar

mattwar commented Mar 5, 2015

Copy link
Copy Markdown
Contributor

Is having this in a remarks section good enough? I don't think anyone will ever see these unless they are reading the source.

@pdelvo

pdelvo commented Mar 5, 2015

Copy link
Copy Markdown
Contributor Author

I think the question should be: Why is Visual Studios rendering of xml docs that minimal? You can really put a lot of work into them just to be disappointed about the number of tags VS actually supports. Why doesn't the remarks section show up? Why isn't list, code supported? I think that would really improve productivity if the documentation tool tip would support that. I will move that sentence into the summary element when I get home but that really feels like an workaround.

@pdelvo

pdelvo commented Mar 5, 2015

Copy link
Copy Markdown
Contributor Author

I moved the text into the summary. I might open an issue for better support for advanced xml docs.

@pharring

Copy link
Copy Markdown
Contributor

@dotnet-bot retest this, please.

@pharring

Copy link
Copy Markdown
Contributor

@gafter I seem to remember you had an idea for how to address the general problem of tagging/enforcing public interfaces 'for internal implementation only'?

@gafter

gafter commented Mar 24, 2015

Copy link
Copy Markdown
Member

@pharring Yes, and it was not approved either as a language change or as a compiler change. There will be an analyzer that helps enforce it.

@pharring

Copy link
Copy Markdown
Contributor

Ah, I see it right there on ISymbol: InternalImplementationOnlyAttribute. And it's enforced via an analyzer.
Is the attribute on the base interface sufficient for the analyzer to catch all cases?

@gafter gafter self-assigned this Mar 24, 2015
@gafter gafter added this to the 1.0 (stable) milestone Mar 24, 2015
@gafter

gafter commented Mar 24, 2015

Copy link
Copy Markdown
Member

@mattwar Do you prefer this to the version where it was in the remarks section.

We will also have an analyzer to "enforce" this.

@gafter gafter added the 4 - In Review A fix for the issue is submitted for review. label Mar 24, 2015
@pharring

Copy link
Copy Markdown
Contributor

Once again answering my own question: yes. The unit tests for the analyzer check the derived cases.
If this passes verification, then it's safe to merge.

@gafter

gafter commented Mar 24, 2015

Copy link
Copy Markdown
Member

@pharring I believe we are going to get rid of the attribute and just enforce it for these types directly. See #1510.

@sharwell

Copy link
Copy Markdown
Contributor

❗ I do not think we should be adding a large amount of text to the summary line for commonly used types for a condition that developers would rarely (at best) consider violating. If this was SHFB-style documentation, it would be inside the <remarks> section, and wrapped:

/// <remarks>
/// <note type="implement">
/// <para>{warning here}</para>
/// </note>
/// </remarks>

Regardless of the exact form, I believe this should be in the <remarks> section, and placed below any remarks related to valid use(s) of the type.

@sharwell

Copy link
Copy Markdown
Contributor

It makes me sad to think how little of the information I wrote in the documentation for CoreTaskExtensions is ever exposed to users. 😢

@gafter

gafter commented Mar 31, 2015

Copy link
Copy Markdown
Member

@pdelvo Can you please put this back in the section (as described in the corresponding issue)?

@pdelvo

pdelvo commented Mar 31, 2015

Copy link
Copy Markdown
Contributor Author

I pushed my old version again with the docs being in the remarks section.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

❓ This interface is internal. Does it still need this note?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Don't care.

@sharwell

Copy link
Copy Markdown
Contributor

It's too bad we can't use the inheritdoc or token constructs from Sandcastle Help File Builder to simplify this (or more importantly, make sure that an update to the text gets propagated to each location where it is used). 😞

gafter added a commit that referenced this pull request Mar 31, 2015
Add docs for ISymbol and its derived interfaces that say that they shoul...
@gafter
gafter merged commit 5fbb52e into dotnet:master Mar 31, 2015
@gafter gafter removed the 4 - In Review A fix for the issue is submitted for review. label Mar 31, 2015
dibarbet pushed a commit that referenced this pull request Aug 18, 2026
Dispose workspaces at the end of RunAsync
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-Compilers Concept-API This issue involves adding, removing, clarification, or modification of an API. Tenet-Compatibility Violation of forwards/backwards compatibility in a design-time piece.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Docs for ISymbol and its derived interfaces should say that they are not to be extended outside Roslyn

6 participants