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.

Sign up for GitHub

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

Ignore nil tags and convert symbol ones #53

Merged
merged 1 commit into from
Oct 26, 2017
Merged

Ignore nil tags and convert symbol ones #53

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions lib/datadog/statsd.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ def port=(port) #:nodoc:

def tags=(tags) #:nodoc:
raise ArgumentError, 'tags must be a Array<String>' unless tags.nil? or tags.is_a? Array
@tags = (tags || []).map {|tag| escape_tag_content(tag)}
@tags = (tags || []).compact.map! {|tag| escape_tag_content(tag)}
end

# Sends an increment (count = 1) for the given stat to the statsd server.
Expand Down Expand Up @@ -358,10 +358,11 @@ def escape_event_content(msg)
end

def escape_tag_content(tag)
remove_pipes(tag).gsub COMMA, BLANK
remove_pipes(tag.to_s).gsub COMMA, BLANK
end

def escape_tag_content!(tag)
tag = tag.to_s
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 can make this tag = tag.to_s if tag.is_a?(Symbol) to avoid stupid cases like numbers or complex objects

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need to check for those, the documentation clearly specifies that tags should be an array of strings. I'm ok with supporting tags because it used to work with 1.x and the change is small, but people reading the documentation will/should likely use strings.

Actually, I think this change is not needed with #51.

tag.gsub!(PIPE, BLANK)
tag.gsub!(COMMA, BLANK)
tag
Expand Down
16 changes: 16 additions & 0 deletions spec/statsd_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,22 @@ class Datadog::Statsd
it 'should reject non-array tags' do
lambda { @statsd.tags = 'tsdfs' }.must_raise ArgumentError
end

it 'ignore nil tags' do
@statsd.tags = ['tag1', nil, 'tag2']
@statsd.tags.must_equal %w[tag1 tag2]
end

it 'converts symbols to strings' do
@statsd.tags = [:tag1, :tag2]
@statsd.tags.must_equal %w[tag1 tag2]
end

it 'assigns regular tags' do
tags = %w[tag1 tag2]
@statsd.tags = tags
@statsd.tags.must_equal tags
end
end

describe "#increment" do
Expand Down