Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#3017 closed enhancement (fixed)

[patch] unit tests showing humanize intcomma handling floats

Reported by: Chris Beaven Owned by: Adrian Holovaty
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 Chris Beaven 10 years ago.
commaseparate.2.patch (2.1 KB) - added by Chris Beaven 10 years ago.
humanize.diff (894 bytes) - added by Russell Keith-Magee 10 years ago.
unit test for humanize.intcomma

Download all attachments as: .zip

Change History (11)

Changed 10 years ago by Chris Beaven

Attachment: commaseparate.patch added

comment:1 Changed 10 years ago by Chris Beaven

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 10 years ago by Chris Beaven

Attachment: commaseparate.2.patch added

comment:2 Changed 10 years ago by Chris Beaven

Triage Stage: UnreviewedReady for checkin

comment:3 Changed 10 years ago by Malcolm Tredinnick

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 10 years ago by Russell Keith-Magee

Attachment: humanize.diff added

unit test for humanize.intcomma

comment:4 Changed 10 years ago by Russell Keith-Magee

Triage Stage: Ready for checkinDesign 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 10 years ago by Chris Beaven

Resolution: worksforme
Status: newclosed

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 10 years ago by Chris Beaven

Resolution: worksforme
Status: closedreopened
Summary: [patch] Humanize intcomma should handle floats[patch] unit tests showing humanize intcomma handling floats
Triage Stage: Design decision neededReady 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 10 years ago by Malcolm Tredinnick

Resolution: fixed
Status: reopenedclosed

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

comment:8 Changed 10 years ago by Malcolm Tredinnick

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.

Note: See TracTickets for help on using tickets.
Back to Top