Code

Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#3017 closed enhancement (fixed)

[patch] unit tests showing humanize intcomma handling floats

Reported by: SmileyChris Owned by: adrian
Component: Contrib apps Version:
Severity: normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

The intcomma filter should handle floats, too.

Of course, the name doesn't make much sense in that case, so I have renamed to commaseparate (while keeping backwards compatibility).

Attachments (3)

commaseparate.patch (1.9 KB) - added by SmileyChris 7 years ago.
commaseparate.2.patch (2.1 KB) - added by SmileyChris 7 years ago.
humanize.diff (894 bytes) - added by russellm 7 years ago.
unit test for humanize.intcomma

Download all attachments as: .zip

Change History (11)

Changed 7 years ago by SmileyChris

comment:1 Changed 7 years ago by SmileyChris

On a related side note, there's one spelling mistake in a docstring somewhere which misspells separate as "seperate".

I can't be bothered opening a ticket for something that negligible, but feel free to fix it as part of this one ;)

Changed 7 years ago by SmileyChris

comment:2 Changed 7 years ago by SmileyChris

  • Triage Stage changed from Unreviewed to Ready for checkin

comment:3 Changed 7 years ago by mtredinnick

I'm going to leave this one until after 0.96 (which will be "real soon now"), just to avoid any confusion with the renaming change (I realise this is backwards compatible; good thinking there). Between 0.96 and 1.0, the list of things people will have to look out for will grow and we'll include this in that.

Changed 7 years ago by russellm

unit test for humanize.intcomma

comment:4 Changed 7 years ago by russellm

  • Triage Stage changed from Ready for checkin to Design decision needed

Are you sure this change is required? I added a unit test for float separation (diff attached), and it worked fine as-is.

Can you provide a specific test case where the existing intcomma filter fails for floats?

comment:5 Changed 7 years ago by SmileyChris

  • Resolution set to worksforme
  • Status changed from new to closed

I'm quite confused also - I can see the current method works perfectly with floats so I'm not sure why I even wrote this patch.

comment:6 Changed 7 years ago by SmileyChris

  • Resolution worksforme deleted
  • Status changed from closed to reopened
  • Summary changed from [patch] Humanize intcomma should handle floats to [patch] unit tests showing humanize intcomma handling floats
  • Triage Stage changed from Design decision needed to Ready for checkin

Actually, the unit test may as well go in there. I opened a new ticket regarding renaming the filter to something more sane: #3884

comment:7 Changed 7 years ago by mtredinnick

  • Resolution set to fixed
  • Status changed from reopened to closed

(In [4880]) Fixed #3017 -- Added tests for humanize filter.

comment:8 Changed 7 years ago by mtredinnick

Russell: I changed one of the tests (the one with a long float) before committing, because it was vulnerable to internal floating-point representation problems. I only caught it because it failed for me.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.