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

Add conditional for when formatting metric name symbol #297

Merged
merged 3 commits into from
Nov 4, 2024

Conversation

rayz
Copy link
Contributor

@rayz rayz commented Oct 25, 2024

Fixes an issue where {Symbol}.name breaks in RUBY_VERSION < 3.

Was not caught in CI since we didn't use symbols in our testing so I've added a new test.

@@ -38,7 +38,11 @@ def global_tags
attr_reader :tag_serializer

def formatted_metric_name(metric_name)
formatted = Symbol === metric_name ? metric_name.name : metric_name.to_s
if Symbol === metric_name && metric_name.respond_to?(:name)

Choose a reason for hiding this comment

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

Code Quality Violation

Suggested change
if Symbol === metric_name && metric_name.respond_to?(:name)
if metric_name.is_a?(Symbol) && metric_name.respond_to?(:name)
Do not use === (...read more)

The case equality operator === in Ruby is used to test equality within a when clause of a case statement. However, it's often considered a bad practice to use this operator explicitly outside of a case statement. This is because its behavior can be quite unpredictable and confusing, as it behaves differently for different classes.

The use of the === operator can lead to code that is harder to read and understand. It's also potentially prone to bugs, as it might not behave as expected with certain objects. Therefore, it's recommended to avoid the explicit use of the === operator.

Instead of using the === operator, it's better to use more explicit methods that clearly indicate what you're trying to achieve. For example, if you're trying to check if a string matches a regular expression, you can use the match? method. If you want to check if an object is an instance of a certain class, you can use the is_a? method. These methods are much more clear and straightforward, leading to better, more maintainable code.

View in Datadog  Leave us feedback  Documentation

Copy link
Member

Choose a reason for hiding this comment

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

Minor: Since this if will always be true or false for a given Ruby version, it may be worth introducing a new method that's conditionally defined for that Ruby; e.g. something like

if RUBY_VERSION < '3'
  def metric_name_to_string(metric_name)
    Symbol === metric_name ? metric_name.name : metric_name.to_s
  end
else
  def metric_name_to_string(metric_name)
    metric_name.to_s
  end
end

...this way each Ruby gets its own correct version of the code and no need to keep checking.

@rayz rayz marked this pull request as ready for review November 4, 2024 15:17
@rayz rayz requested a review from a team as a code owner November 4, 2024 15:17
@rayz rayz requested a review from ivoanjo November 4, 2024 15:17
Copy link
Member

@ivoanjo ivoanjo left a comment

Choose a reason for hiding this comment

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

👍 LGTM

spec/statsd/serialization/stat_serializer_spec.rb Outdated Show resolved Hide resolved
Copy link

@DDuongNguyen DDuongNguyen left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏼

@rayz rayz merged commit 5f962c9 into master Nov 4, 2024
14 checks passed
end
else
def metric_name_to_string(metric_name)
Symbol === metric_name ? metric_name.name : metric_name.to_s

Choose a reason for hiding this comment

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

Code Quality Violation

Suggested change
Symbol === metric_name ? metric_name.name : metric_name.to_s
metric_name.is_a?(Symbol) ? metric_name.name : metric_name.to_s
Do not use === (...read more)

The case equality operator === in Ruby is used to test equality within a when clause of a case statement. However, it's often considered a bad practice to use this operator explicitly outside of a case statement. This is because its behavior can be quite unpredictable and confusing, as it behaves differently for different classes.

The use of the === operator can lead to code that is harder to read and understand. It's also potentially prone to bugs, as it might not behave as expected with certain objects. Therefore, it's recommended to avoid the explicit use of the === operator.

Instead of using the === operator, it's better to use more explicit methods that clearly indicate what you're trying to achieve. For example, if you're trying to check if a string matches a regular expression, you can use the match? method. If you want to check if an object is an instance of a certain class, you can use the is_a? method. These methods are much more clear and straightforward, leading to better, more maintainable code.

View in Datadog  Leave us feedback  Documentation

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.

3 participants