On Mon, Feb 6, 2017 at 10:32 AM, Michael Osipov <micha...@apache.org> wrote:
> Am 2017-02-06 um 19:23 schrieb Nathan Bubna: > >> On Mon, Feb 6, 2017 at 9:43 AM, Michael Osipov <micha...@apache.org> >> wrote: >> >>> 7) check object for a length() or size() method, if so return whether it >>> >>>> returns 0, but I agree with Alex Fedotov that we could skip those >>>> methods if we already took care for strings and collections. >>>> >>>> >>> What happened to array#length? You completely missed that out. >>> I would not drop #length() and #size(), you'd ultimately fail with >>> javax.naming.directory.Attribute#size() or >>> javax.naming.directory.Attributes#size(). >>> There are likely more examples to have this. >>> >> >> >> i don't think it hurts to keep them, most users won't often get that far, >> i >> think. >> > > I did not say we should drop them, I said that they are crucial in some > situations. Dropping them would be wrong. Yup. I was agreeing. :) > 8) If directive.if.tostring.check = false, stop here and return true (*) >> >>> >>>> 9) check object for a toString() method, and return whether the string >>>> is non-null and non-empty >>>> >>>> The 6th step won't be reached very often... >>>> >>>> (*) the old configuration parameter was directive.if.tostring.nullchec >>>> k, >>>> but for consistency with the new behavior regarding empty strings, my >>>> proposal is to rename it like this. I don't consider that backward >>>> compatibility is important since all collections are handled above in >>>> the chain. >>>> >>>> >>> 9) is somewhat confusing because #toString() is never null unless you >>> override it and return a custom string. Moreover, how will a #toString() >>> guarantee you that an object is logically empty? It can't, see >>> javax.naming.directory.SearchResult#getAttributes(). >>> >>> At best, this would be false by default in 2.0. >>> >> >> ... >> >> I still back this check. Velocity is for templating, text output, i.e. a >> display language. Velocity-specific classes (including some VelocityTools) >> have had cause in the past to return null or empty strings specifically >> because of this check and that toString() is regularly central to >> rendering >> objects. While it cannot guarantee emptiness for every object out there, >> it >> is a sensible check. The goal here is not perfection, but to catch common >> cases, and due to history, i believe this is a common one. >> > > Again, there might be cases this is necessary though I cannot make up one > from the top of my head. I am just saying that this should not be a default > setting and people must know what they enable at the end. My concern was backward compatibility, which, while not necessary, is still quite valuable. There are existing VelocityTools that rely on this, after all. Given all the ways to avoid getting to this check, i don't see the compelling reason to toggle the default here. Though, as always, i will defer to those doing actual work right now. :) Just chiming in with my two bits.
