-
Notifications
You must be signed in to change notification settings - Fork 62
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
Enable style cop #111
Enable style cop #111
Conversation
…ject configuration
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! Just a couple of questions!
@@ -22,5 +23,10 @@ | |||
<Version>4.3.0</Version> | |||
</PackageReference> | |||
</ItemGroup> | |||
|
|||
<ItemGroup> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we get to run this stuff run in the CI?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the changes done in src/StatsdClient/StatsdClient.csproj
warnings are generated when compiling.
We should catch StyleCop warnings in AppVeyor as I used /warnaserror
flag at: https://github.com/DataDog/dogstatsd-csharp-client/pull/111/files#diff-180360612c6b8c4ed830919bbb4dd459R10.
I will test it once previous PRs will be merged (AppVeyor tests are failing).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch!
There is an existing issue that prevents /warnaserror
to work as expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
This build https://ci.appveyor.com/project/Datadog/dogstatsd-csharp-client/builds/32467628 is an example of Style Cop issues caught by the CI.
SocketSender.Send(MaxUDPPacketSize, command, | ||
encodedCommand => UDPSocket.SendTo(encodedCommand, encodedCommand.Length, SocketFlags.None, IPEndpoint)); | ||
|
||
return Environment.GetEnvironmentVariable(StatsdConfig.DD_AGENT_HOST_ENV_VAR); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed we renamed this constant (ENTITY_ID_INTERNAL_TAG_KEY = "dd.internal.entity_id"
): https://github.com/DataDog/dogstatsd-csharp-client/pull/111/files#diff-d0ebbcc771918be301e58f78757158cbL15, should DD_AGENT_HOST_ENV_VAR
be renamed as well? If so, did we miss any others?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I renamed ENTITY_ID_INTERNAL_TAG_KEY
as this field was private.
DD_AGENT_HOST_ENV_VAR
is a public field and cannot be renamed without breaking client code. Instead, I marked the field as obsolete in this PR:
https://github.com/DataDog/dogstatsd-csharp-client/pull/113/files#diff-d30af27dd1bd06efdef90392c5163883R48-R55
Also, noticed there are a couple of conflicts, they're both trivial. |
…csharp-client into olivierg/AddStyleCop # Conflicts: # src/StatsdClient/AdvancedStatsConfig.cs # src/StatsdClient/Dogstatsd.cs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
This PR enables style cop.
Note:
StatsdClient.Tests
.olivierg/AddTelemetry
will be merged