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

Reduce warnings across projects #1808

Merged
merged 27 commits into from
Jun 18, 2020
Merged

Conversation

bdebaere
Copy link
Contributor

@bdebaere bdebaere commented Jun 13, 2020

Fix CA1200: replace cref="\w: with cref=".
Fix xUnit2017: replace Assert.IsTrue(Contains) with Assert.Contains.
Fix CA1063: implement Dispose correctly.
Fix CA1309: use StringComparison.Ordinal.
Fix CA1507: use nameof instead of string literal.
Fix CS1658: replace `1 with generic parameter in comments.
Fix SA1500: adjust StringsClassGenerator.ttinclude to span multiple lines.

Copy link
Contributor

@paulodero paulodero left a comment

Choose a reason for hiding this comment

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

:shipit:

Copy link
Contributor

@KenitoInc KenitoInc left a comment

Choose a reason for hiding this comment

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

Thanks @bdebaere for this PR. It will greatly assist in cleaning up the codebase. A few change requests and we will be good to merge.

return Microsoft.OData.Edm.EdmRes.GetString(Microsoft.OData.Edm.EdmRes.Constructable_VocabularyAnnotationMustHaveTarget);
}
}

/// <summary>
/// A string like "An entity type or a collection of an entity type or a complex type is expected."
/// A string like "An entity type or a collection of an entity type is expected."
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did this string change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file is auto generated. I simply executed the tt script. I don't know what else to tell you. :)

{
return;
}

Copy link
Member

Choose a reason for hiding this comment

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

why remove this codes? It's unsafe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See https://docs.microsoft.com/en-us/visualstudio/code-quality/ca1063?view=vs-2019 on how to implement IDisposable correctly.

The Dispose method should have the code below, and only the code below. Any additional cleanup must be performed by Dispose(bool disposing).

        public void Dispose()
        {
            this.Dispose(true);
            GC.SuppressFinalize(this);
        }

Copy link
Contributor

Choose a reason for hiding this comment

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

@gathogojr @g2mula What do you think?

/// </summary>
internal sealed class EdmRes {
internal const string EdmPrimitive_UnexpectedKind = "EdmPrimitive_UnexpectedKind";
internal const string EdmPath_UnexpectedKind = "EdmPath_UnexpectedKind";
internal const string Annotations_TypeMismatch = "Annotations_TypeMismatch";
internal const string Constructable_VocabularyAnnotationMustHaveTarget = "Constructable_VocabularyAnnotationMustHaveTarget";
internal const string Constructable_EntityTypeOrCollectionOfEntityTypeOrComplexTypeExpected = "Constructable_EntityTypeOrCollectionOfEntityTypeOrComplexTypeExpected";
internal const string Constructable_EntityTypeOrCollectionOfEntityTypeExpected = "Constructable_EntityTypeOrCollectionOfEntityTypeExpected";
Copy link
Member

Choose a reason for hiding this comment

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

Constructable_EntityTypeOrCollectionOfEntityTypeExpected [](start = 30, length = 56)

It's weird, we don't need to change Microsoft.OData.Edm.txt file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm unfamiliar with these files. I simply executed the tt script. If that is not enough, let me know!

@xuzhg
Copy link
Member

xuzhg commented Jun 16, 2020

    internal static string EdmPrimitive_UnexpectedKind {

This file and Microsoft.OData.Edm.cs is autogenerated from "Microsoft.OData.Edm.txt" and "......tt" file,

Do you right click the "... tt" file and auto generate it? #Resolved


Refers to: src/Microsoft.OData.Edm/Parameterized.Microsoft.OData.Edm.cs:21 in 5b22550. [](commit_id = 5b22550, deletion_comment = True)

@xuzhg
Copy link
Member

xuzhg commented Jun 16, 2020

using System.Globalization;

Typically, we should ignore "Microsoft.OData.Edm.cs" and "Paramer.....Microsoft.OData.Edm.s" for the styleCop.

We can do like this:

  1. add "autogenerate" into head
  2. ignore it in the config

If we don't want to ignore them, please modify the template file and make sure the output meets the stylecop.


Refers to: src/Microsoft.OData.Edm/Microsoft.OData.Edm.cs:11 in 5b22550. [](commit_id = 5b22550, deletion_comment = False)

@@ -207,8 +207,10 @@ namespace <#= Configuration.ResourceClassNamespace #> {
private void GenerateAsProperty(string id)
{
#>
internal static string <#= id #> {
get {
Copy link
Member

Choose a reason for hiding this comment

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

cool

@xuzhg
Copy link
Member

xuzhg commented Jun 16, 2020

It looks good. I'd like to merge it ASAP. However, there's one change confusing me. I left a comment at ODataInputContext.cs, please take a look and address that.

@xuzhg xuzhg added the Ready for review Use this label if a pull request is ready to be reviewed label Jun 16, 2020
@KenitoInc
Copy link
Contributor

@bdebaere Since you are adding more commits, Ping me when done so that we can review this PR

@bdebaere
Copy link
Contributor Author

@bdebaere Since you are adding more commits, Ping me when done so that we can review this PR

@KenitoInc I'm done now.

@xuzhg xuzhg merged commit c6876f2 into OData:master Jun 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready for review Use this label if a pull request is ready to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants