Alright, I'll document the current behavior. On Tue, Aug 19, 2025 at 1:43 AM Kevin Rushforth <kevin.rushfo...@oracle.com> wrote:
> I am not sure it is worth changing the behavior either. It allowing null > were ambiguous (or harmful), then it might be worth it. As it is, it seems > better to just document the current behavior. > > -- Kevin > > > On 8/18/2025 2:34 PM, John Hendrikx wrote: > > I think it may come down to whether rejecting `null` would be helpful for > users of these constructors to find problems in their code. I agree that > the DateTimeStringConverter is not really a chained constructor with a > clear (and public) master constructor that is documented to allow `null` > for certain defaults. It is more a style of programming used throughout FX > to provide many constructors (Image is also like that) instead of a Builder > or static factory method. > > I could make arguments that it would make programmatic use easier, but > that never stopped me from rejecting nulls in my own code. Callers then > should just pick their own defaults, like: > > new DateTimeConverter(locale == null ? Locale.DEFAULT : locale, > pattern == null ? "ddMMyy" : pattern) > > Personally, I think this is not worth changing, but I do think it should > be documented. Rejecting nulls now will definitely require a CSR and is > not backwards compatible :/ > > --John > > On 18/08/2025 22:41, Nir Lisker wrote: > > But I think I see what you mean better now, for example, there is (Locale >> locale, String pattern) and (String pattern). If you intending to pass >> `null` as Locale, why not just use the pattern-only constructor? I think >> that still could make sense, > > > OK, but what about `pattern` being null in either? > > On Mon, Aug 18, 2025 at 11:30 PM John Hendrikx <john.hendr...@gmail.com> > wrote: > >> If certain patterns make no sense, then I think the master constructor >> should reject them (if there is one, or perhaps one should be created as a >> private one). >> >> For example, if there is a width/height constructor, but I only specify >> width and not height, then I think its fine to reject (null, height) and >> (width, null) while accepting (null, null) and (width, height). >> >> But I think I see what you mean better now, for example, there is (Locale >> locale, String pattern) and (String pattern). If you intending to pass >> `null` as Locale, why not just use the pattern-only constructor? I think >> that still could make sense, especially locale is say coming from a >> configuration file or some such. If the constructor `null` here, then it >> makes it easier to use it programmatically (ie. if my configuration has a >> Locale then I pass that, if it is null, I pass that -- compare that to >> having to check for Locale being null and then having to select the correct >> constructor... >> >> --John >> >> On 18/08/2025 21:00, Nir Lisker wrote: >> >> Aren't these constructors chained? >> >> >> Not all of them. There are 3 tangential creation paths for >> DateTimeStringConverter and subclasses: >> 1. Through dateStyle/timeStyle ints with an optional locale that creates >> the DateFormat with `DateFormat.getDateTimeInstance(dateStyle, timeStyle, >> locale)` (similar for subclasses). There are 4 constructors here for the >> combinations of: none, only style(s), only locale, all. >> 2. Through a String pattern with an optional locale that creates the >> DateFormat with `new SimpleDateFormat(pattern, locale)`. If pattern is >> null, it uses path 1 above with default styles and the optional locale. If >> you wanted to use a pattern to create this converter, passing null makes >> little sense. It could be surprising to get a "defaults" converter when you >> intended to customize the format with your own pattern to begin with. I >> imagine that if you couldn't get a pattern, you'd use your own int styles >> as a replacement. >> 3. Through a DateFormat that is used directly. If it's null, it checks if >> the pattern is null (which it will always be), in which case it uses the >> defaults of path 1 again (with the default locale since it's not optional >> here). I find it odd here too to pass null and rely on the >> "defaults" converter. >> >> NumberStringConverter and its subclasses are similar. They have path 2 >> with 4 combinations: none, only pattern, and locale, and both. And they >> also have path 3 that falls on the default if null. >> >> On Mon, Aug 18, 2025 at 9:06 PM John Hendrikx <john.hendr...@gmail.com> >> wrote: >> >>> Aren't these constructors chained? I believe it is quite common >>> practice to use nulls when calling a chained constructor to get a default >>> for that parameter. If a certain type of convenience constructor is >>> missing, a caller can pass in `null` for the parameter they'd like >>> defaulted. It's not too far-fetched to allow this **if** there is a >>> constructor where this parameter is omitted and is assigned a default. >>> >>> If anything, the constructors IMHO should document that for certain >>> parameters passing in `null` results in a specific default. >>> >>> --John >>> On 18/08/2025 19:46, Nir Lisker wrote: >>> >>> Hi all, >>> >>> In DateTimeStringConverter, NumberStringConverter, and their subclasses, >>> null parameters sent to the constructors are internally converted to >>> default values. This is not specified, but it's how the implementation >>> behaves. I'm working on some changes there and was thinking about changing >>> the behavior to throw NPEs as it makes no sense to pass null into these and >>> it's probably a bug more than anything. >>> >>> The LocalDate/Time converters specified the null-friendly behavior in >>> their docs even though it doesn't make much sense there either. >>> >>> Are we allowed to change this? >>> >>> - Nir >>> >>> >
