It looks like neither HSSFWorkbook nor XSSFWorkbook have sheet iterators defined for them.

I'm not sure what's preferred in this case, upcasting all iterators to Iterator<Sheet> or leaving the iterators in their more specific sheet type. The former is how XSSFWorkbook.iterator is currently implemented. The latter is similar to how XSSFSheet.rowIterator is currently implemented.

POI devs, what's preferred here?


============

ss.usermodel.Workbook:
public interface Workbook extends Closeable, Iterable {
    Iterator<? extends Sheet> iterator();
}

xssf.usermodel.XSSFWorkbook:
public class XSSFWorkbook extends POIXMLDocument implements Workbook {
    @Override
    public Iterator<XSSFSheet> iterator() {
        return sheets.iterator();
    }

hssf.usermodel.HSSFWorkbook:
public final class HSSFWorkbook extends POIDocument implements Workbook {
    @Override
    public Iterator<HSSFSheet> iterator() {
        return _sheets.iterator();
    }

xssf.streaming.SXSSFWorkbook:
public class SXSSFWorkbook implements Workbook {
    @Override
    public Iterator<SXSSFSheet> iterator() {
        return new SXSSFSheetIterator(_wb.iterator());
    }
}

=================

ss.usermodel.Workbook:
public interface Workbook extends Closeable, Iterable {
    Iterator<Sheet> iterator();
}

xssf.usermodel.XSSFWorkbook:
public class XSSFWorkbook extends POIXMLDocument implements Workbook {
    @Override
    public Iterator<Sheet> iterator() {
        return (Iterator<Sheet>)(Iterator<? extends Sheet>) sheets.iterator();
    }

hssf.usermodel.HSSFWorkbook:
public final class HSSFWorkbook extends POIDocument implements Workbook {
    @Override
    public Iterator<Sheet> iterator() {
        return (Iterator<Sheet>)(Iterator<? extends Sheet>) _sheets.iterator();
    }

xssf.streaming.SXSSFWorkbook:
public class SXSSFWorkbook implements Workbook {
    @Override
    public Iterator<Sheet> iterator() {
        Iterator<SXSSFSheet> it = new SXSSFSheetIterator(_wb.iterator());
        return (Iterator<Sheet>)(Iterator<? extends Sheet>) it;
    }
}

I had the same intention with X/HSLF [1], but this ends in some interesting generics definitions ... it works now, but I still think that some declarations could be simpler ..

so I guess, I would do it as in org.apache.poi.sl.usermodel.ShapeContainer with a parameterized interface

[1] http://stackoverflow.com/questions/29440337/java-generics-parameterized-class-vs-typed-method

Mimicking what you wrote in r1692593:
org.apache.poi.sl.usermodel.ShapeContainer:  https://svn.apache.org/viewvc/poi/trunk/src/java/org/apache/poi/sl/usermodel/ShapeContainer.java?limit_changes=0&r1=1692593&r2=1692592&pathrev=1692593
org.apache.poi.xslf.usermodel.XSLFShapeContainer: https://svn.apache.org/viewvc/poi/trunk/src/ooxml/java/org/apache/poi/xslf/usermodel/XSLFShapeContainer.java?r1=1410315&r2=1692593&pathrev=1692593

Is this what you were referring to?


ss.usermodel.Workbook:
public interface Workbook<T extends Sheet> extends Closeable, Iterable<T> {
    Iterator<T> iterator();
}

xssf.usermodel.XSSFWorkbook:
public class XSSFWorkbook extends POIXMLDocument implements Workbook<XSSFSheet> {
    @Override
    public Iterator<XSSFSheet> iterator() {
        return sheets.iterator();
    }

hssf.usermodel.HSSFWorkbook:
public final class HSSFWorkbook extends POIDocument implements Workbook<HSSFSheet> {
    @Override
    public Iterator<HSSFSheet> iterator() {
        return _sheets.iterator();
    }

xssf.streaming.SXSSFWorkbook:
public class SXSSFWorkbook implements Workbook<SXSSFSheet> {
    @Override
    public Iterator<SXSSFSheet> iterator() {
        return new SXSSFSheetIterator(_wb.iterator());
    }
}

I suppose I could go as far as writing a new interface:
public interface SheetContainer<T extends Sheet> extends Iterable<T>, but I currently don't have any additional methods that a SheetContainer would need to implement beyond what an Iterable requires.

This might be getting a bit out of my Java comfort zone here--I'm used to Python where generics/templates aren't needed because everything is duck typed.

Suggestions?

I don't think we can do the Workbook<T> trick. I had a play with that quite recently. The killer is that Eclipse (+friends) all start complaining when you write something like "Workbook wb = WorkbookFactory.create(file)", claiming that you should be giving it a generics type. We don't want to do that, as it defeats the whole point of telling everyone to use the SS interfaces!

We certainly could do 
Workbook:  Iterator<? extends Sheet> getSheetIterator();
HSSFWorkbook:  Iterator<HSSFSheet> getSheetIterator();

That works fine, and mirrors what we have for rows and cells. It's the iterable version that I think will have to remain "Workbook implements Iterable<Sheet>" and "HSSFWorkbook implements Workbook, Iterable<Sheet>", unless someone can come up with a cunning workaround!

Created attachment 33066 [details]
patch to make all workbooks implement Iterable<Sheet>

The XSSFWorkbook.iterator breaks that pattern used by Sheet.iterator and Row.iterator

Unfortunately, XSSFWorkbook has had this method for nearly 7 years (r700472 https://svn.apache.org/viewvc?view=revision&revision=700472).
Nonetheless, I think deprecating the old iterator is a step in the right direction, allowing developers to write for loops like

for (Sheet sh : workbook) {
    for (Row row : sh) {
        for (Cell cell : row) {
            System.out.println(cell);
        }
    }
}

I've provided test cases and documentation to help users transition their code to the new Iterator<Sheet> iterator() interface (previously Iterator<XSSFSheet> iterator().

Note: this patch will break backwards compatibility with existing code.

Created attachment 33067 [details]
site documentation updates

Updated quick-guide to include for-each sheet iteration in the attached patch.

These are the changes that developers would need to make with attachment 33066 [details]


final XSSFWorkbook wb = new XSSFWorkbook();
wb.createSheet();

// =====================================================================
// Case 1: Existing code uses XSSFSheet for-each loop
// =====================================================================
// Original code (no longer valid)
for (XSSFSheet sh : wb) {
    sh.createRow(0);
}

// Option A:
for (XSSFSheet sh : (Iterable<XSSFSheet>) (Iterable<? extends Sheet>) wb) {
    sh.createRow(0);
}

// Option B (preferred for new code):
for (Sheet sh : wb) {
    sh.createRow(0);
}

// =====================================================================
// Case 2: Existing code creates an iterator variable
// =====================================================================
// Original code (no longer valid)
Iterator<XSSFSheet> it = wb.iterator();
XSSFSheet sh = it.next();
sh.createRow(0);

// Option A:
Iterator<XSSFSheet> it = (Iterator<XSSFSheet>) (Iterator<? extends Sheet>) wb.iterator();
XSSFSheet sh = it.next();
sh.createRow(0);

// Option B (deprecated, but a quick-fix)
@SuppressWarnings("deprecation")
Iterator<XSSFSheet> it = wb.xssfSheetIterator();
XSSFSheet sh = it.next();
sh.createRow(0);

// Option C (preferred for new code):
Iterator<Sheet> it = wb.iterator();
Sheet sh = it.next();
sh.createRow(0);

Created attachment 33068 [details]
site documentation updates

fix quick-guide whitespace

Looks good to me, thanks for this

I think that once we've got the build green again from the forbidden APIs check changes, we should be fine to apply this as-is. Please remind us in 1-2 weeks if we forget!

As of r1702321, build 841 <https://builds.apache.org/job/POI/841/>, the build server is green again.

Thanks for this, and for your patience!

Code change applied in r1703573, site in r1703574.