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 >>
