Bug 55330 - [PATCH] Enhance Type Safety of org.apache.poi.ss.usermodel Interface APIs
Summary: [PATCH] Enhance Type Safety of org.apache.poi.ss.usermodel Interface APIs
Status: NEW
Alias: None
Product: POI
Classification: Unclassified
Component: SS Common (show other bugs)
Version: 3.9-FINAL
Hardware: All All
: P2 enhancement (vote)
Target Milestone: ---
Assignee: POI Developers List
URL:
Keywords: PatchAvailable
Depends on:
Blocks: 59836
  Show dependency tree
 
Reported: 2013-07-31 03:07 UTC by Ryan O'Meara
Modified: 2016-10-09 11:38 UTC (History)
1 user (show)



Attachments
Page Margins Enumeration (29.36 KB, patch)
2013-08-01 05:41 UTC, Ryan O'Meara
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryan O'Meara 2013-07-31 03:07:32 UTC
(Filed under POI Overall component as this applies to the user model shared by both HSSF and XSSF)

There are various API definitions in interfaces from the org.apache.poi.ss.usermodel package which could be made significantly more type-safe by replacing short/int arguments with various Java enumerations. Some of these methods have been type safe for particular implementations in a way which could be expanded to include both implementations (both meaning the xssf and hssf implementations of the interfaces)

The most prominent example(s) of this are (all from the org.apache.poi.ss.usermodel package):

- Cell
   - get/setCellType
- CellStyle
   - get/setAlignment
   - get/setVerticalAlignment
   - get/setBorderLeft
   - get/setBorderRight
   - get/setBorderTop
   - get/setBorderBottom
   - get/setFillPattern
Font
   - get/setTypeOffset
   - get/setUnderline
   - get/setCharSet
   - get/setBoldweight
Sheet
   - get/setMargin

The proposal for this enhancement is to create/fully utilize enumerations for the various sets of numeric constants which are meant to be used with these methods to create alternate APIs, and to deprecate the APIs which are not type safe in favor of the new ones.
Comment 1 Ryan O'Meara 2013-07-31 03:11:12 UTC
I am willing to work on creating a patch for this enhancement if there is general agreement it is a desired enhancement
Comment 2 Nick Burch 2013-07-31 10:13:52 UTC
Two comments and one query:
 - Before defining the Enum, there'd need to be some time spent looking over the File Format Documentation to ensure that all the different options are actually defined, and none have been missed
 - As well as this, there would need to be a check done of all the test documents we have to ensure that no extra values in common use

 - How would we handle a file where the value written in it when reading wasn't one of the expected ones? (Could be slightly invalid, from a different program, reserved value in use etc - real world files are often messy!)


It might be worth picking one of the smaller, more self contained cases from the list, and trying to work up a patch for just that. A patch will mean code to review and discuss (rather than just in the abstract), and would offer more of a chance to see the best ways to handle the comments+question above.
Comment 3 Ryan O'Meara 2013-07-31 13:33:16 UTC
I meant to ask if you'd want to see a patch for all or a patch for one to see if you liked how I went about it - guess that answers that question! I will work up a patch for one of the better defined cases (I'm thinking margins - there are only so many valid options for that one) and we can discuss based on that.

There are a couple of different techniques I'd suggest for the invalid value scenario - we can either include an "unknown value" enum (throwing an exception if its attempted as an input) to cover those cases, or we can throw an exception during parsing if we are confident that we have covered all values. 

It shouldn't be too hard t nail down the values for most of these - they generally say right in the docs "must be on of (list of constants)", which is what made me think enumerations would be a better fit for this area. If the function is only meant to accept a certain range of values, it would be better to use Java's built-in type safety to help support it instead of accidentally letting values through or throwing exceptions at run time.

So, current plan: 

- Scrub the file format documentation for the possible valid margin arguments
- Check the example/test documents for commonly used margin values
- Create an enumeration of those values, containing the data necessary to convert them to the file format data
- Create a patch and attach to this bug for discussion

By the way, at the moment, I have checked out the source using Git. Will a patch created via Git work, or should I check out the source via SVN and create a patch using that?
Comment 4 Nick Burch 2013-07-31 14:20:04 UTC
(In reply to Ryan O'Meara from comment #3)
> By the way, at the moment, I have checked out the source using Git. Will a
> patch created via Git work, or should I check out the source via SVN and
> create a patch using that?

The canonical source tree for Apache POI is the SVN one. The git repo is a read-only copy.

Any patches will need to be applied using SVN, but unless you're doing something unusual a diff generated by Git is processable by modern copies of SVN. Please send in single git patches for meaningful chunks - you may need to squash some smaller commits into bigger ones so we end up with things of a suitable size to review and apply
Comment 5 Ryan O'Meara 2013-07-31 14:42:04 UTC
OK, sounds good. My planned commit size was one commit per type enumerated (for example, margins) including the added enumeration, modifications, and any unit tests.
Comment 6 Ryan O'Meara 2013-08-01 05:41:14 UTC
Here is a patch which adds type safe operations for page margins. I started with margins as the constants used there do not go directly into the generated file - they are just used to determine which function is called on internal handler routines, so there is less risk of unanticipated values which need to be handled by the APIs

The patch adds an enumeration and the new API calls, deprecates the old API calls and constants, and duplicates the tests which existed for the old API calls for the new ones
Comment 7 Ryan O'Meara 2013-08-01 05:41:51 UTC
Created attachment 30653 [details]
Page Margins Enumeration