DataTable string formatters no longer applied conditionally based on value

[lsmith]

Bug introduced in 0b48532#src/datatable/js/body.js

When a column is declared with a string formatter and an emptyCellValue, the emptyCellValue should be applied when the field value for that column is null, undefined, or the empty string. This is missing test coverage, which explains the regression.

This is manifest in the DataTable with checkboxes example.

[lsmith]

Ping @Satyam

[patjameson]

So it seems that you would like to have the emptyCellValue completely override the formatter?

The way I figured it would work is that the empty cell value changes the value of the cell, and then the formatter formats the value (using {value} if desired). This is how it seems to work now. Because there lacks a {value} in the formatter, the formatter overwrites the value of the cell.

However, I believe I have a fix that does what you would like, if you want to check that out. Hopefully I understand what you would like done here.

[lsmith]

Yes, the emptyCellValue should supply the entire cell content. That way it can be used generically with or without formatters, and with string or function formatters. For example:

columns: [
    { key: 'salary', formatter: '${value}k', emptyCellValue: 'n/a' },
    ...
]

You don't want cells with "$n/ak". If emptyCellValue were effectively a default for {value}, you would need to use a function formatter to avoid this.

Cool to hear you have a fix. Looking forward to seeing it!

[patjameson]

Oh I figured everyone on this page could see the reference I made in my commit. Here's a link if you want to check it out before I fully test/write more tests and then do a pull request. It seems to work with your n/a example.

patjameson@24c4f57

[Satyam]

Hi guys. Sorry @lsmith for the delay in responding to that ping, I was out of town, completely disconnected for a week.

The problem is with a 'further optimization' we did. We thought we could move some processing up to the creation of the row template so we are inserting the value of the string formatter right into the row template so it applies to all rows, no matter what the value might be. There is not much to be done when processing each of the rows since the template used is fixed by then.

Regarding the order of operations discussed somewhere above, the formatting function (whether given as a named function or an actual function) should be processed first and then the emptyCellValue should be applied. If the emptyCellValue were to be applied before the formatting function, the latter would find the value already changed and would not be able to discriminate in between, say, null and "", null meaning 'no value' while "" might be the equivalent of ''this page intentionally left blank".

[lsmith]

@Satyam Your recollection of the "further optimization" sounds familiar! Regarding order of operations, I didn't get the impression that anyone was suggesting the emptyCellValue be applied before formatters.

That said, it does look like @patjameson's commit does apply emptyCellValue before formatters, which I agree with @Satyam isn't the right way about it. I would recommend the fix include:

• "optimization" in _createRowTemplate where string formatters are added to the template is removed.
• Formatters collection gets a new formatter that returns the col.formatter string template after value substitution.
• _setColumnsFormatterFn tests for string type for col.formatter and if not found, sets col._formatterFn to this new Formatters formatter.
• _createRowHTML is left alone to look exclusively for col._formatterFn and call that if present, then flowing out of the formatter conditional block to supply missing content with col.emptyCellValue if present.
• Tests are added to reveal the regression of course

[Satyam]

I like the idea of adding a string template formatter so that everything goes through the same _formatterFn. While drafting that idea, I found this in an earlier version:

https://github.com/yui/yui3/blob/v3.8.0/src/datatable/js/body.js#L685

Which is supported by this:

http://yuilibrary.com/yui/docs/datatable/#formatters-vs-empty

Now, what happens when a field returns null, a very normal situation in a SQL environment. Since it is not undefined, the string template will be processed using "null" as the value and, of course, the emptyCellValue will not be used. Does this make sense?

[Satyam]

@lsmith I did as suggested, except that the new formatter for string templates is not located in the datatable-formatters module but in datatable-body since the former is an optional dependency. It is still in the Formatters hash of functions.

The point that still bugs me is the use of the formatting function only when the value is not undefined while the emptyCellValue is used when the value is undefined, null or an empty string. This leaves the null and empty string values not covered by either the formatter or the emptyCellValue. I'm specially concerned about null values as a regular SQL database would return which would demand custom code at either end to convert those nulls into undefineds, which doesn't really make much sense.

I haven't pushed the new code yet because of this concern and because I'm still working on the tests and the API docs.

[Satyam]

In the tests, I added column h. You can see its definition here:

https://github.com/Satyam/yui3-1/blob/issue843/src/datatable/tests/unit/assets/datatable-body-tests.js#L310

and the values loaded into it right below.

As the asserts in the test show:

https://github.com/Satyam/yui3-1/blob/issue843/src/datatable/tests/unit/assets/datatable-body-tests.js#L367

"((null))" results when the field is null. This doesn't look right. May I apply the formatting template when the value is not undefined nor null?

[Satyam]

I haven't commented on a field containing an empty string because I'm not clear myself about how it should be considered. Is it empty or intentionally left blank? However, for a cell with both a string formatter and an emptyCellValue set, unless the string template is the identity formatter (i.e.: "{value}"), the emptyCellValue would never get called since the string formatter would be applied resulting in something that is not an empty string. In the example cited above, it would result in "(())" which would not be replaced by emptyCellValue.

Whatever interpretation you want to give to what is empty and what is not, this seems inconsistent to me.

[lsmith]

I don't remember the rationale for making string formatters accept null and the empty string as template values. There might not have been one, but there's an outside chance I might have had a reason and it was documented in a commit comment.

As it stands now, I'm ok with changing the rules for string formatters to disregard undefined, null, and empty string. With a note in the HISTORY.md, of course.

[Satyam]

In:

https://github.com/yui/yui3/commits/master/src/datatable/js/body.js

If you go back to Feb 13th, 2012 and Jan 29th there are some comments. It would seem that initially null was not considered as empty and, two weeks later, when added in one place, the other was forgotten. In the very first commit, only "" was empty, but I couldn't find where undefined was added.

Anyway, I was thinking of applying the formatter whenever the condition for emptyCellValue does not apply and even making that configurable. As it happens, it is faster to do if ([undefined, null, ""].indexOf(value) !== -1) than doing the three separate comparissons which would even allow them to be configurable. The question is where should I put that array, a datatable attribute, a public property or a protected one? I'll start with a protected property, let me know if you think it would be better otherwise.