Skip to content
This repository has been archived by the owner on Nov 26, 2017. It is now read-only.

Add some new tests for quote. #1733

Merged
merged 1 commit into from
Dec 22, 2012
Merged

Add some new tests for quote. #1733

merged 1 commit into from
Dec 22, 2012

Conversation

elinw
Copy link
Contributor

@elinw elinw commented Dec 3, 2012

These tests document the quote method's handling of data types that are not strings.

@elinw
Copy link
Contributor Author

elinw commented Dec 3, 2012

Hopefully I did this in a way that will work with Andrew's pull request, but this documents the behaviors we were discussing (which were slightly different than I thought) so that we can be sure of having a stable api.

@eddieajau
Copy link
Contributor

What are you concerned might happen if we don't have these tests? To me, it's just testing that some particular values are being correctly converted to strings. We could just as easily do the following to enforce that:

return '\'' . ($escape ? $this->escape((string) $text) : (string) $text) . '\'';

Maybe I'm missing the point?

@elinw
Copy link
Contributor Author

elinw commented Dec 3, 2012

It's not some particular values, it is some particular data types (although it is good to document for example that true goes to '1' not 'true' and that false goes to null and not '0' or 'false') are being converted to strings. Tests are one of the best ways that we have of documenting behavior especially non obvious behaviors and making sure they don't actually change when we change other things. So as we talked about the other day we could have an api that throws exceptions instead but we don't. I mean sure you don't really need float and integer but why not be complete? I was a little bit inspired by the http://php.net/manual/en/function.is-string.php is_string example in its thoroughness.

@eddieajau
Copy link
Contributor

I'm not convinced it's necessary, and I would be inclined to roll them into one test with multiple assertions anyway. If others think it's necessary I'm fine with it.

@elinw
Copy link
Contributor Author

elinw commented Dec 3, 2012

Well yeah a data provider will be the way in the end, but I didn't want to mess up your pr or have yours mess up mine :).

@eddieajau
Copy link
Contributor

Ok. I'd have no issue with a data provide pattern.

@elinw
Copy link
Contributor Author

elinw commented Dec 3, 2012

Okay can I do that after your stuff is merged so it doesn't have to be done twice?

ianmacl pushed a commit that referenced this pull request Dec 22, 2012
Add some new tests for quote.
@ianmacl ianmacl merged commit 89f4b9c into joomla:staging Dec 22, 2012
@elinw elinw deleted the quotetests branch December 23, 2012 13:15
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants