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

Improve doc comments #6284

Merged
merged 2 commits into from
Apr 5, 2021
Merged

Improve doc comments #6284

merged 2 commits into from
Apr 5, 2021

Conversation

gewarren
Copy link
Contributor

@gewarren gewarren commented Mar 19, 2021

cc @ghogen

Contributes to dotnet/dotnet-api-docs#2053.

For doc comment guidelines, see https://github.com/dotnet/dotnet-api-docs/wiki.

/// <remarks>
/// An example value is "5.1.2600.0" for Windows XP.
/// If you don't specify a value, a default value is used.
/// The default value is the minimum supported OS of the .NET Framework, which is "4.10.0.0" for Windows 98 Second Edition.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@danmoseley Should this MSBuild logic be updated since Windows 98 is no longer supported?

Copy link
Member

@danmoseley danmoseley Mar 22, 2021

Choose a reason for hiding this comment

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

This isn't my code/product. I believe this relates to ClickOnce which I don't think (?) is under active development. So my inclination if I owned this would be to not touch functionality unless I have to. I would just update the comment as seems best. It doesn't seem very consequential if it's not quite right.

Copy link
Member

Choose a reason for hiding this comment

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

Team triage: We've seen some PRs from ClickOnce, so it still seems to be under active development. @sujitnayak and @John-Hart can comment on whether they want to make a change, though I agree it doesn't seem too consequential.

Choose a reason for hiding this comment

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

ClickOnce is still supported and in fact, we have updated this recently for .NET Core, but as to which versions of Windows and .NET that are supported or the defaults is more of a ClickOnce Runtime question for either @merriemcgaw or @NikolaMilosavljevic

Copy link
Member

Choose a reason for hiding this comment

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

I stand corrected.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should modify the default supported OS. Let's keep the old value intact. Customers can always modify this on their own.

@Forgind Forgind added merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. and removed merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. labels Mar 29, 2021
@Forgind Forgind added the merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. label Apr 5, 2021
@Forgind Forgind merged commit b7476cc into main Apr 5, 2021
@rainersigwald rainersigwald deleted the gewarren-patch-1 branch April 15, 2021 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants