[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-openjfx-dev
Subject:    Re: Exposed implementation details in DateTimeStringConverter
From:       Nir Lisker <nlisker () gmail ! com>
Date:       2021-01-27 1:37:38
Message-ID: CA+0ynh8buL7bcSorwSG59DRfM=v7gWfYeGgmvmwy683-G9W6Lw () mail ! gmail ! com
[Download RAW message or body]

Yes, sorry, I should have been more clear. Only dateFormat is used during
runtime, the other 4 are used only during construction to create a
DateFormat if one is not given, they shouldn't even be fields. dateFormat
encodes all the information of the converter and has a getter (also
protected access and maybe should be deprecated as well). I don't see how
subclasses could use those even if someone wanted to make a subclass for
some reason.

I filed JDK-8260475 [1].

[1] https://bugs.openjdk.java.net/browse/JDK-8260475

On Wed, Jan 27, 2021 at 12:19 AM Kevin Rushforth <kevin.rushforth@oracle.com>
wrote:

> I presume you mean these three?
>
>      protected final Locale locale;
>      protected final String pattern;
>      protected final DateFormat dateFormat;
>
> and maybe also these two?
>
>      /**
>       * @since JavaFX 8u40
>       */
>      protected final int dateStyle;
>
>      /**
>       * @since JavaFX 8u40
>       */
>      protected final int timeStyle;
>
> Applications can't use them directly (outside a subclass), so the
> questions are: Do subclasses need access to these fields, and do we
> intend that an application be able to subclass the parent class with
> their own implementation?
>
> The fields are used by the two subclasses that we provide in the same
> package, so unless we expect applications or third-party libraries to
> access them, which I don't think we do, it doesn't seem like they should
> be part of the public API.
>
> We should either file a bug to document them or to deprecate them
> (probably for removal, but I wouldn't be in too big a big hurry to
> remove them). I suspect deprecating them is fine.
>
> -- Kevin
>
>
> On 1/26/2021 12:08 PM, Nir Lisker wrote:
> > Hi,
> >
> > DateTimeStringConverter exposes final protected fields that are used for
> > internal implementation. These fields should have been private or
> > package-private. The reasons are:
> > 1. A comment above them refers to them as private fields.
> > 2. They have no documentation attached.
> > 3. They are useless to the user (except for the DateFormat one) as some
> of
> > their values are unused, depending on the construction method, and they
> are
> > discarded after it.
> > 4. There is a DateFormat getter method specifically for this field only,
> > which is the only field that is relevant during the life of the class. It
> > makes no sense to expose the field in addition.
> >
> > I suggest deprecating these fields for removal from the public API. There
> > are minimal risks that these fields are being read by the user through
> > direct access.
> >
> > - Nir
>
>
[prev in list] [next in list] [prev in thread] [next in thread]