-
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
Fix style cop warnings for statsdClient.Tests #112
Fix style cop warnings for statsdClient.Tests #112
Conversation
…er the parameter span across multiple lines
…r should be placed on its own line.
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! Asked a couple of questions, but didn't spot any blockers!
// =-=-=-=- COUNTER -=-=-=-= | ||
|
||
[Test] | ||
public void send_increase_counter_by_x() | ||
public void Send_increase_counter_by_x() |
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.
we use CamelCase almost everywhere, should we consider dropping the snake_case and renaming these?
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.
Ideally we should use CamelCase everywhere. I didn't do it as there are 211 functions to rename (I used the Replace All
feature to capitalize the first letter).
Do you think I should rename these functions?
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.
Let's rename those in a separate PR. I have the same issue with some java tests.
@@ -71,7 +78,8 @@ public void UDSStatsdServerName() | |||
Environment.SetEnvironmentVariable(StatsdConfig.DD_AGENT_HOST_ENV_VAR, null); | |||
Assert.AreEqual("server1", GetUDSStatsdServerName(CreateUDSConfig("server1"))); | |||
|
|||
Environment.SetEnvironmentVariable(StatsdConfig.DD_AGENT_HOST_ENV_VAR, | |||
Environment.SetEnvironmentVariable( | |||
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 made a comment in #111 about how you renamed one of the constants there to use s different naming convention. I'm not sure why we changed that constant and not others, I just want to mention that in case you think we should rename all constants.
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.
The main difference is the visibility. DD_AGENT_HOST_ENV_VAR
is public and cannot be renamed without a breaking change: #111 (comment)
[Test] | ||
public void _udp_listener_sanity_test() | ||
public void Udp_listener_sanity_test() |
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.
ditto about CamelCase vs snake_case. I'm not sure what the convention is, just want to make sure if we stick to snake_case it's for good reason.
…sharp-client into olivierg/FixStyleCopWarningForStatsdClient.Tests # Conflicts: # tests/StatsdClient.Tests/Bufferize/StatsBufferizeTests.cs # tests/StatsdClient.Tests/CommandIntegrationTests.cs # tests/StatsdClient.Tests/DogstatsdStaticApiTests.cs # tests/StatsdClient.Tests/StatsdConfigurationTests.cs # tests/StatsdClient.Tests/UdpListener.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.
Thanks for answering all my questions. 📦
Fix style cop warnings for StatsdClient.Tests
Note: This PR can be merged only when #111 will be merged.