Be more lenient with trim()

[bruno-c]

Allowing for null and undefined values passed to trim() to return empty string.

I'm not sure if you would rather deal with this in the functions calling trim() but this will at least allow some flexibility handling attributes that may be null or contain a string.

[wyuenho]

Mmmm. I'm inclined to reject this because I want to keep the behavior of trim as close to String.prototype.trim as possible. You can't do nul.trim(), that makes no sense. Is there a real problem that this PR is trying to solve?

[bruno-c]

You're right of course, I should have spent more time explaining this.

The issue has to do with null values. The DatetimeFormatter does not appear to handle null values correctly. Because trim() is called on all the raw values, an exception is raised.

An example of a failing test:

  it(".fromRaw() ignores null values", function () {

    var formatter = new Backgrid.NumberFormatter();
    expect(formatter.fromRaw(null)).toBe(null);

  });

Or a higher level test, for DateCell:

  it("handles a null value", function () {
    var cell = new Backgrid.DateCell({
      model: new Backbone.Model({
        date: null
      }),
      column: {
        name: "date",
        cell: "date"
      }
    });
    cell.render();
    expect(cell.$el.hasClass("date-cell")).toBe(true);
  });

My patch avoids the problem by adding a special case to trim in order to avoid the exception being raised TypeError: String.prototype.trim called on null or undefined

There's probably a better place to handle it, checking for null / undefined before going to trim(). Or name the function differently :)

[wyuenho]

You want null displayed in cells?

[bruno-c]

No -- I want nothing displayed in those cells. Right now it throws an exception. How else would you indicate the absence of a value?

[wyuenho]

Ok I need you to do a couple of things. First, rebase your branch against 0.2.0-wip, I like your test cases, but I think the fix should be inside the DatetimeFormatter._convert as it is the only place where trim is causing problems.

See what you can do about this line. https://github.com/wyuenho/backgrid/blob/0.2.0-wip/src/formatter.js#L193

When you are done them I'll merge. Thanks.

[wyuenho]

Hi @bruno-c, are you still working on this?

[bruno-c]

@wyuenho I would love to take care of this immediately, but I am currently a bit swamped with day-to-day work. I'll try to find some time later on today, if that works for you.

[wyuenho]

Yep no problem. I'm trying to release a maintenance release of 0.1.2 that's why I was asking. Thanks a bunch in advance!

[bruno-c]

Closing this pull request - the new one is #43