Fix #19663, text provided to colorize() shouldn't raise exception if None #671

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants
@johnraz

johnraz commented Jan 24, 2013

See #670 for the initial pull request and https://code.djangoproject.com/ticket/19663 for details.

@johnraz

This comment has been minimized.

Show comment Hide comment
@johnraz

johnraz Jan 26, 2013

@charettes : could you please review this one ? I'm not sure you saw that I created a new one and closed the previous one ;)

johnraz commented Jan 26, 2013

@charettes : could you please review this one ? I'm not sure you saw that I created a new one and closed the previous one ;)

@charettes

This comment has been minimized.

Show comment Hide comment
@charettes

charettes Jan 26, 2013

Member

Fix and test looks good, utils tests pass on python 2.7.3 and 3.2.3 (SQLite), will mark RFC.

Member

charettes commented Jan 26, 2013

Fix and test looks good, utils tests pass on python 2.7.3 and 3.2.3 (SQLite), will mark RFC.

@charettes

This comment has been minimized.

Show comment Hide comment
@charettes

charettes Jan 26, 2013

Member

Actually the ticket is already marked as RFC. Since I'm not a core committer you'll have to wait until one finds the time to commit your fix.

Member

charettes commented Jan 26, 2013

Actually the ticket is already marked as RFC. Since I'm not a core committer you'll have to wait until one finds the time to commit your fix.

+ def test_colorize_none_text(self):
+ self.assertTrue(colorize(text=None))
+
+ self.assertTrue(colorize(text=None, opts=('noreset')))

This comment has been minimized.

Show comment Hide comment
@claudep

claudep Jan 26, 2013

Member

Is there a reason why you assertTrue instead of assertEqual with colorize result?

@claudep

claudep Jan 26, 2013

Member

Is there a reason why you assertTrue instead of assertEqual with colorize result?

This comment has been minimized.

Show comment Hide comment
@johnraz

johnraz Jan 27, 2013

The reason is that the purpose of the test is to check that colorize won't raise any exception and not that it returns a specific value.
I was looking for an assertNoException but such a thing doesn't exist afaik.
Still, testing the return of the colorize function would be a nice thing but shouldn't it be done in another test?
Didn't bother to look if it already exists btw.
I can change it to assertEqual if you find it more robust / adequate.

@johnraz

johnraz Jan 27, 2013

The reason is that the purpose of the test is to check that colorize won't raise any exception and not that it returns a specific value.
I was looking for an assertNoException but such a thing doesn't exist afaik.
Still, testing the return of the colorize function would be a nice thing but shouldn't it be done in another test?
Didn't bother to look if it already exists btw.
I can change it to assertEqual if you find it more robust / adequate.

This comment has been minimized.

Show comment Hide comment
@charettes

charettes Jan 27, 2013

Member

Maybe this approach would be more appropriate:

try:
    colorize(text=None)
except TypeError:
    self.fail("colorise shouldn't raise a type error when text=None")

I also suggest that you add a reference to the trac ticket in the test_colorize_none_text doc, it's faster than running a blame.

@charettes

charettes Jan 27, 2013

Member

Maybe this approach would be more appropriate:

try:
    colorize(text=None)
except TypeError:
    self.fail("colorise shouldn't raise a type error when text=None")

I also suggest that you add a reference to the trac ticket in the test_colorize_none_text doc, it's faster than running a blame.

This comment has been minimized.

Show comment Hide comment
@johnraz

johnraz Jan 27, 2013

Yes it's definitely cleaner this way, good to know that you can self.fail() !
Ack for the trac ticket ref in the doc.
I don't have access to my dev box right now but will update my pull request with all of this as soon as I get my hands on it.

@johnraz

johnraz Jan 27, 2013

Yes it's definitely cleaner this way, good to know that you can self.fail() !
Ack for the trac ticket ref in the doc.
I don't have access to my dev box right now but will update my pull request with all of this as soon as I get my hands on it.

This comment has been minimized.

Show comment Hide comment
@claudep

This comment has been minimized.

Show comment Hide comment
@claudep

claudep Feb 1, 2013

Member

Thanks, fixed in 04141c5

Member

claudep commented Feb 1, 2013

Thanks, fixed in 04141c5

@claudep claudep closed this Feb 1, 2013

nanuxbe added a commit to nanuxbe/django that referenced this pull request Jul 2, 2016

ramiro pushed a commit to ramiro/django that referenced this pull request Jul 2, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment