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 behavior of content_tag when use with content that is not a string #1563

Merged
merged 2 commits into from Jan 29, 2014

Conversation

Projects
None yet
3 participants
@tyabe
Contributor

tyabe commented Jan 29, 2014

If you pass a number in content_tag, It is considered as a codepoint.
maybe this behavior is not intended in many cases.
(ex: If you pass an array of numeric options to the select_tag)

This PR is change it to be treated as a string.
What do you think to that?

before:
  content_tag(:p, 97) #=> <p>a</p>

after:
  content_tag(:p, 97) #=> <p>97</p>
@ujifgc

This comment has been minimized.

Show comment
Hide comment
@ujifgc

ujifgc Jan 29, 2014

Member

Makes sense, though I would not add tests for classes like Hash and Array, I could not imagine these use cases.

Member

ujifgc commented Jan 29, 2014

Makes sense, though I would not add tests for classes like Hash and Array, I could not imagine these use cases.

@namusyaka

This comment has been minimized.

Show comment
Hide comment
@namusyaka

namusyaka Jan 29, 2014

Member

@ujifgc BTW, can you look at this failure?
I think this code should be something like files += app.app_obj.dependencies if app.app_obj.respond_to?(:dependencies) if mounted_app should support Sinatra.

Member

namusyaka commented Jan 29, 2014

@ujifgc BTW, can you look at this failure?
I think this code should be something like files += app.app_obj.dependencies if app.app_obj.respond_to?(:dependencies) if mounted_app should support Sinatra.

@tyabe

This comment has been minimized.

Show comment
Hide comment
@tyabe

tyabe Jan 29, 2014

Contributor

@ujifgc I agree with you. I modifying the test case.

Contributor

tyabe commented Jan 29, 2014

@ujifgc I agree with you. I modifying the test case.

@ujifgc

This comment has been minimized.

Show comment
Hide comment
@ujifgc

ujifgc Jan 29, 2014

Member

@namusyaka I created an issue, I will look at it later, it's not critical.

Member

ujifgc commented Jan 29, 2014

@namusyaka I created an issue, I will look at it later, it's not critical.

Fix test case for content_tag
exclude cases that would not be used
@tyabe

This comment has been minimized.

Show comment
Hide comment
@tyabe

tyabe Jan 29, 2014

Contributor

I exclude cases that would not be used.

Contributor

tyabe commented Jan 29, 2014

I exclude cases that would not be used.

@ujifgc

This comment has been minimized.

Show comment
Hide comment
@ujifgc

ujifgc Jan 29, 2014

Member

Looks good.

Member

ujifgc commented Jan 29, 2014

Looks good.

@namusyaka

This comment has been minimized.

Show comment
Hide comment
@namusyaka

namusyaka Jan 29, 2014

Member

Great, thank you!

Member

namusyaka commented Jan 29, 2014

Great, thank you!

namusyaka added a commit that referenced this pull request Jan 29, 2014

Merge pull request #1563 from tyabe/fix_content_tag
Fix behavior of content_tag when use with content that is not a string

@namusyaka namusyaka merged commit a1aeaae into padrino:master Jan 29, 2014

1 check passed

default The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment