[Bug 59836] New: Replace primitives with enums

classic Classic list List threaded Threaded
33 messages Options
12
Reply | Threaded
Open this post in threaded view
|

[Bug 59836] New: Replace primitives with enums

Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=59836

            Bug ID: 59836
           Summary: Replace primitives with enums
           Product: POI
           Version: 3.15-dev
          Hardware: PC
                OS: Linux
            Status: NEW
          Severity: enhancement
          Priority: P2
         Component: SS Common
          Assignee: [hidden email]
          Reporter: [hidden email]

This is a aggregate bug for recent changes in classes from using short or int
to using enums with codes.

Benefits:
* self-explanatory usage rather than needing to list the valid constants in the
javadocs
* easier to distinguish arguments in function signatures (not just a 5 ints)
* clearer error messages, no functions needed to convert code to string
* free range checking (less boiler plate code that does the same)
* type safety

Drawbacks:
* some permgen overhead
* type checking execution overhead

Overall, this seems like a good thing.

In effort to maintain backwards compatibility, the following is suggested:

If there is currently
int getSomething();
void setSomething(int);

Then we should add
enum getSomethingEnum();
void setSomething(Enum);

And deprecate
void setSomething(int);
int getSomething();

It's a bit tricky handling getSomething/getSomethingEnum while maintaining
backwards compatibility because at some point the signature will abruptly
change. Both getters should be offered for a while with a note encouraging
users to use the constants or enums rather than using literal values. This is
good coding practice anyway to not hard-code literal values, but is especially
important with regard to backwards compatibility here.

At least 2 releases later, but not later than POI 4.0:
Delete void setSomething(int);
Delete int getSomething();
Rename enum getSomethingEnum(); to enum getSomething();

In the end, we will have
enum getSomething();
void setSomething(enum);
as the only two methods.

--
You are receiving this mail because:
You are the assignee for the bug.

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 59836] Tracker: Replace primitives with enums

Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=59836

Javen O'Neal <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
          Component|SS Common                   |POI Overall
         Depends on|                            |59264, 58671, 59833, 59790,
                   |                            |58636, 58190, 59791
            Summary|Replace primitives with     |Tracker: Replace primitives
                   |enums                       |with enums

--
You are receiving this mail because:
You are the assignee for the bug.

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 59836] Tracker: Replace primitives with enums

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=59836

Javen O'Neal <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Depends on|                            |59837

--
You are receiving this mail because:
You are the assignee for the bug.

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [Bug 59836] New: Replace primitives with enums

Mark Murphy
In reply to this post by Bugzilla from bugzilla@apache.org
Maybe we should have a standard enum template that defines everything that
belongs in the enum class. It would be easiest if enums could inherit from
each other, or if they weren't final classes, but that doesn't work. I am
thinking of some conversion from and to the simple type underlaying it, but
that doesn't work for the binary formats. Maybe the better way is to have a
conversion class that takes the various enums and converts them from/to the
various simple types.

On Sun, Jul 10, 2016 at 2:51 AM, <[hidden email]> wrote:

> https://bz.apache.org/bugzilla/show_bug.cgi?id=59836
>
>             Bug ID: 59836
>            Summary: Replace primitives with enums
>            Product: POI
>            Version: 3.15-dev
>           Hardware: PC
>                 OS: Linux
>             Status: NEW
>           Severity: enhancement
>           Priority: P2
>          Component: SS Common
>           Assignee: [hidden email]
>           Reporter: [hidden email]
>
> This is a aggregate bug for recent changes in classes from using short or
> int
> to using enums with codes.
>
> Benefits:
> * self-explanatory usage rather than needing to list the valid constants
> in the
> javadocs
> * easier to distinguish arguments in function signatures (not just a 5
> ints)
> * clearer error messages, no functions needed to convert code to string
> * free range checking (less boiler plate code that does the same)
> * type safety
>
> Drawbacks:
> * some permgen overhead
> * type checking execution overhead
>
> Overall, this seems like a good thing.
>
> In effort to maintain backwards compatibility, the following is suggested:
>
> If there is currently
> int getSomething();
> void setSomething(int);
>
> Then we should add
> enum getSomethingEnum();
> void setSomething(Enum);
>
> And deprecate
> void setSomething(int);
> int getSomething();
>
> It's a bit tricky handling getSomething/getSomethingEnum while maintaining
> backwards compatibility because at some point the signature will abruptly
> change. Both getters should be offered for a while with a note encouraging
> users to use the constants or enums rather than using literal values. This
> is
> good coding practice anyway to not hard-code literal values, but is
> especially
> important with regard to backwards compatibility here.
>
> At least 2 releases later, but not later than POI 4.0:
> Delete void setSomething(int);
> Delete int getSomething();
> Rename enum getSomethingEnum(); to enum getSomething();
>
> In the end, we will have
> enum getSomething();
> void setSomething(enum);
> as the only two methods.
>
> --
> You are receiving this mail because:
> You are the assignee for the bug.
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>
>
Reply | Threaded
Open this post in threaded view
|

Re: [Bug 59836] New: Replace primitives with enums

Javen O'Neal
Unfortunately, Java language doesn't allow enums to be subclassed or
created via a factory.

Enums need to be statically defined at run time so that equality checks and
switch statements can be resolved at compile time rather than run time.
This is a Java 6 requirement.
On Jul 10, 2016 7:29 AM, "Mark Murphy" <[hidden email]> wrote:

> Maybe we should have a standard enum template that defines everything that
> belongs in the enum class. It would be easiest if enums could inherit from
> each other, or if they weren't final classes, but that doesn't work. I am
> thinking of some conversion from and to the simple type underlaying it, but
> that doesn't work for the binary formats. Maybe the better way is to have a
> conversion class that takes the various enums and converts them from/to the
> various simple types.
>
> On Sun, Jul 10, 2016 at 2:51 AM, <[hidden email]> wrote:
>
> > https://bz.apache.org/bugzilla/show_bug.cgi?id=59836
> >
> >             Bug ID: 59836
> >            Summary: Replace primitives with enums
> >            Product: POI
> >            Version: 3.15-dev
> >           Hardware: PC
> >                 OS: Linux
> >             Status: NEW
> >           Severity: enhancement
> >           Priority: P2
> >          Component: SS Common
> >           Assignee: [hidden email]
> >           Reporter: [hidden email]
> >
> > This is a aggregate bug for recent changes in classes from using short or
> > int
> > to using enums with codes.
> >
> > Benefits:
> > * self-explanatory usage rather than needing to list the valid constants
> > in the
> > javadocs
> > * easier to distinguish arguments in function signatures (not just a 5
> > ints)
> > * clearer error messages, no functions needed to convert code to string
> > * free range checking (less boiler plate code that does the same)
> > * type safety
> >
> > Drawbacks:
> > * some permgen overhead
> > * type checking execution overhead
> >
> > Overall, this seems like a good thing.
> >
> > In effort to maintain backwards compatibility, the following is
> suggested:
> >
> > If there is currently
> > int getSomething();
> > void setSomething(int);
> >
> > Then we should add
> > enum getSomethingEnum();
> > void setSomething(Enum);
> >
> > And deprecate
> > void setSomething(int);
> > int getSomething();
> >
> > It's a bit tricky handling getSomething/getSomethingEnum while
> maintaining
> > backwards compatibility because at some point the signature will abruptly
> > change. Both getters should be offered for a while with a note
> encouraging
> > users to use the constants or enums rather than using literal values.
> This
> > is
> > good coding practice anyway to not hard-code literal values, but is
> > especially
> > important with regard to backwards compatibility here.
> >
> > At least 2 releases later, but not later than POI 4.0:
> > Delete void setSomething(int);
> > Delete int getSomething();
> > Rename enum getSomethingEnum(); to enum getSomething();
> >
> > In the end, we will have
> > enum getSomething();
> > void setSomething(enum);
> > as the only two methods.
> >
> > --
> > You are receiving this mail because:
> > You are the assignee for the bug.
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: [hidden email]
> > For additional commands, e-mail: [hidden email]
> >
> >
>
Reply | Threaded
Open this post in threaded view
|

[Bug 59836] Tracker: Replace primitives with enums

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=59836

Javen O'Neal <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Depends on|                            |59873

--
You are receiving this mail because:
You are the assignee for the bug.

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 59836] Tracker: Replace primitives with enums

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=59836
Bug 59836 depends on bug 59873, which changed state.

Bug 59873 Summary: Replace Hyperlink type int constants with a HyperlinkType enum
https://bz.apache.org/bugzilla/show_bug.cgi?id=59873

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |FIXED

--
You are receiving this mail because:
You are the assignee for the bug.

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 59836] Tracker: Replace primitives with enums

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=59836
Bug 59836 depends on bug 59837, which changed state.

Bug 59837 Summary: Replace CellStyle alignment constants with HorizontalAlignment and VerticalAlignment enums
https://bz.apache.org/bugzilla/show_bug.cgi?id=59837

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |FIXED

--
You are receiving this mail because:
You are the assignee for the bug.

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 59836] Tracker: Replace primitives with enums

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=59836

Javen O'Neal <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Depends on|                            |59877

--
You are receiving this mail because:
You are the assignee for the bug.

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 59836] Tracker: Replace primitives with enums

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=59836
Bug 59836 depends on bug 59791, which changed state.

Bug 59791 Summary: Convert Cell Type to an enum
https://bz.apache.org/bugzilla/show_bug.cgi?id=59791

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEEDINFO                    |RESOLVED
         Resolution|---                         |FIXED

--
You are receiving this mail because:
You are the assignee for the bug.

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 59836] Tracker: Replace primitives with enums

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=59836
Bug 59836 depends on bug 58190, which changed state.

Bug 58190 Summary: [PATCH] The current picture handling uses raw integers for types and index, replace with enum and reference
https://bz.apache.org/bugzilla/show_bug.cgi?id=58190

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEEDINFO                    |RESOLVED
         Resolution|---                         |FIXED

--
You are receiving this mail because:
You are the assignee for the bug.

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 59836] Tracker: Replace primitives with enums

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=59836

Javen O'Neal <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Depends on|                            |60136

--
You are receiving this mail because:
You are the assignee for the bug.

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 59836] Tracker: Replace primitives with enums

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=59836

--- Comment #1 from Javen O'Neal <[hidden email]> ---
Todo: update https://poi.apache.org/spreadsheet/quick-guide.html

--
You are receiving this mail because:
You are the assignee for the bug.

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 59836] Tracker: Replace primitives with enums

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=59836

Javen O'Neal <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Depends on|                            |60187

--
You are receiving this mail because:
You are the assignee for the bug.

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 59836] Tracker: Replace primitives with enums

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=59836
Bug 59836 depends on bug 60187, which changed state.

Bug 60187 Summary: RegionUtil should use new enum style classes
https://bz.apache.org/bugzilla/show_bug.cgi?id=60187

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |FIXED

--
You are receiving this mail because:
You are the assignee for the bug.

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 59836] Tracker: Replace primitives with enums

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=59836

Javen O'Neal <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Depends on|                            |60228

--
You are receiving this mail because:
You are the assignee for the bug.

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 59836] Tracker: Replace primitives with enums

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=59836
Bug 59836 depends on bug 60228, which changed state.

Bug 60228 Summary: Cell.getCellTypeEnum should not be deprecated until Cell.getCellType returns a CellType
https://bz.apache.org/bugzilla/show_bug.cgi?id=60228

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |FIXED

--
You are receiving this mail because:
You are the assignee for the bug.

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 59836] Tracker: Replace primitives with enums

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=59836

Javen O'Neal <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Depends on|                            |55330

--
You are receiving this mail because:
You are the assignee for the bug.

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 59836] Tracker: Replace primitives with enums

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=59836
Bug 59836 depends on bug 60228, which changed state.

Bug 60228 Summary: Cell.getCellTypeEnum should not be deprecated until Cell.getCellType returns a CellType
https://bz.apache.org/bugzilla/show_bug.cgi?id=60228

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |UNCONFIRMED
         Resolution|FIXED                       |---

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 59836] Tracker: Replace primitives with enums

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=59836
Bug 59836 depends on bug 60228, which changed state.

Bug 60228 Summary: Cell.getCellTypeEnum should not be deprecated until Cell.getCellType returns a CellType
https://bz.apache.org/bugzilla/show_bug.cgi?id=60228

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|---                         |FIXED

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

12