poi-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From bugzi...@apache.org
Subject [Bug 60826] Add initial support for SAX-like read-only .xlsb parser
Date Fri, 17 Mar 2017 10:11:39 GMT
https://bz.apache.org/bugzilla/show_bug.cgi?id=60826

Tim Allison <tallison@mitre.org> changed:

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

--- Comment #7 from Tim Allison <tallison@mitre.org> ---
Javen, 
  Many, many thanks for your careful code review!  Changes made r1787320.
Let me know if you see anything else worth changing.

Details:

Global: Replace 3.15-beta3 with 3.16-beta3
        >Done

XSSFBHeaderFooter: headerFooterHelper should either be a static final member,
or even better, a non-instantiable (static) class with static methods.
        >Changed to static final.  Y, we should make that a non-instantiable
static class.  I was working with what was already in POI.

Docs: use https URLs when possible 
        >I found one in package.html, should I also change them in the license
headers?

Docs: spell out full org.apache.poi... rather than abbreviating to o.a.p
        >Done.

XSSFBRelation: can getContents be moved up to a superclass (POIXMLRelation)?
There isn't any logic here that's specific to XSSFB.
        >Done, and removed from XSSFRelation

XSSFBRichStr: we should try to make this implement the RichString interface
later.
        >Agreed.

XSSFBRecordType: lookup can have constant lookup time if we use a Map at the
expense of some memory.
        >Done.

XSSFBRichTextString: add @org.apache.poi.util.NotImplemented annotation to
stubbed functions that haven't been implemented yet.
        >Done.

Global: add @since 3.16-beta3 javadoc to all new classes
        >Done.

XSSFBCellHeader: formatAddressAsString can be implemented using new
CellAddress(int row, int col).formatAsString().
        >As part of premature optimization, I was trying to cut down on new
objects.  Turns out that was vestigial to an earlier draft, and I've removed
it.

XSSFBSharedStringsTable: change constructor Javadoc to @since 3.16-beta3.
        >deleted. Relying on @since 3.16-beta3 at class level.

XSSFBSharedStringsTable: should getItems return an unmodifiableList or a copy
of the list?
        >Yes...always the tradeoff of security vs efficiency.  I've modified it
to make a copy.

XSSFBCellRange: "public static final int length" is the standard modifier
order. See http://stackoverflow.com/a/10299123/2683399 for order
        >Did a global replace in xssf.  I suspect we could do this across the
project. 

XSSFBCellRange: this should have tighter integration with
o.a.p.ss.util.AreaReference in the future, but AreaReference probably needs
some cleanup first.
        >Agreed.

XSSFBCommentsTable: should the natural ordering of CellAddresses be implemented
into the CellAddress class itself instead as CellAddressComparator inner class?
Or perhaps as a standalone class if both row-major and column-major ordering is
needed.
        >Duh.  CellAddress already naturally sorts row-major.  I removed the
CellAddressComparator in XSSFBCommentsTable. 

XSSFEventBasedExcelExtractor: any reason for elevating visibility from private?
Was this just a quick fix to get the code to work? Is the performance
acceptable if you add accessors to private members? If not, does protected
work?
        >All back to private with access via getters.

XSSFBEventBasedExcelExtractor: should we log caught and suppressed exceptions
to the POILogger rather than stderr?
        >Done in both XSSFBEvent... and XSSFEvent...

TestExtractorFactory: replace assertTrue(String.contains(String)) with
POITestCase.assertContains(String haystack, String needle)
        >Done throughout TestExtractorFactory

TestXSSFBReader: import assertContains from POITestCase rather than redefining.
        >Gah.  Thank you.  Done.

TestXSSFBEventBasedExcelExtractor: testShapes uses String.indexOf(String) > -1.
Consider using or adding something to POITestCase to make the purpose clearer
        >Used POITestCase and fixed TestXSSFEventBasedExcelExtractor throughout
as well.

-- 
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org


Mime
View raw message