-
Notifications
You must be signed in to change notification settings - Fork 137
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
Ignore nil tags and convert symbol ones #53
Ignore nil tags and convert symbol ones #53
Conversation
end | ||
|
||
def escape_tag_content!(tag) | ||
tag = tag.to_s |
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 can make this tag = tag.to_s if tag.is_a?(Symbol)
to avoid stupid cases like numbers or complex objects
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 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.
5222e07
to
7f8d8b7
Compare
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 the contribution!
I left a small comment, LGTM apart from that.
end | ||
|
||
def escape_tag_content!(tag) | ||
tag = tag.to_s |
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 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.
@degemer well, we do read documentation, but there is a breaking change that slows down from upgrade applications to the 3.*. However the major version bump perfectly fits to introduce breaking change, so either option would be great to implement:
what you think? |
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.
Hi @pschambacher, I'm going to merge this, thanks for your contribution
🙇 |
@degemer
When upgrading to version 3.0, we got a couple errors. In previous versions (at least v1), you could pass nil tags that would be ignored and symbol tags that would be converted to strings.
This brings the functionality back.