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

Added comments and fixed stylecop warnings #1547

Merged
merged 7 commits into from
Oct 12, 2017
Merged

Conversation

nmetulev
Copy link
Contributor

@nmetulev nmetulev commented Oct 11, 2017

Issue: #1451

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[ ] Feature
[x] Code style update (formatting)
[x] Refactoring (no functional changes, no api changes)
[ ] Build or CI related changes
[ ] Documentation content changes
[ ] Sample app changes
[ ] Other... Please describe:

What is the current behavior?

Too many compiler warnings caused by missing comments and other stylecop formatting rules

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tested code with current supported SDKs
  • Docs have been added / updated (for bug fixes / features)
  • Sample in sample app has been added / updated (for bug fixes / features)
  • Tests for the changes have been added (for bug fixes / features) (if applicable)

What is the new behavior?

Warnings for missing comments and stylecope warnings have been removed

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

Copy link
Contributor

@dotMorten dotMorten left a comment

Choose a reason for hiding this comment

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

Changes are ok, but the code summary wording used on properties and constructors is often not according to the standard

@@ -486,14 +495,29 @@ public static void AddAnimation(Storyboard storyboard, DependencyObject element,
/// </summary>
public struct Proj
{
/// <summary>
/// Position of an item
Copy link
Contributor

Choose a reason for hiding this comment

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

The correct property wording is "Gets or sets the [blah]" (same for the ones below)

@@ -34,11 +34,15 @@ namespace Microsoft.Toolkit.Uwp.UI.Controls
[ContentProperty(Name = "Content")]
public partial class Expander : ContentControl
{
/// <summary>
/// Default constructor for the Expander control
Copy link
Contributor

Choose a reason for hiding this comment

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

The proper constructor wording is Initializes a new instance of the <see cref="Expander" /> class @

@@ -22,12 +22,21 @@ namespace Microsoft.Toolkit.Uwp.UI.Controls.TextToolbarFormats
/// </summary>
public abstract class Formatter
{
/// <summary>
/// Constructor for the <see cref="Formatter"/> class
Copy link
Contributor

Choose a reason for hiding this comment

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

Non-standard constructor wording

@@ -14,28 +14,39 @@

namespace Microsoft.Toolkit.Uwp.UI.Controls.TextToolbarFormats.MarkDown
{
/// <summary>
/// Class defining actions for MarkDown
Copy link
Contributor

Choose a reason for hiding this comment

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

wording

public class MarkDownButtonActions : ButtonActions
{
/// <summary>
/// Constructor for the <see cref="MarkDownButtonActions"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

wording

@nmetulev nmetulev merged commit 92ca411 into master Oct 12, 2017
@nmetulev nmetulev deleted the nmetulev/cleanup branch October 12, 2017 03:12
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.

4 participants