Worksheet __getitem__

Issue #604 wontfix
Peter Pul created an issue

While figuring out how __getitem__ worked on Worksheet with slices. And I wonder if key.start and key.stop need a sanity check. My proposal;

            if not key.start or not key.stop:
                raise InsufficientCoordinatesException("Failure msg")
            # Add maybe even more checks?

I wrote some extra tests on current behaviour;

    def test_getitem_sliced(self, Worksheet):
        ws = Worksheet(Workbook())
        cells = ws["A1":"E2"]
        expected = (
            (ws['A1'], ws['B1'], ws['C1'], ws['D1'], ws['E1']),
            (ws['A2'], ws['B2'], ws['C2'], ws['D2'], ws['E2']),
        )
        assert tuple(cells) == expected

    def test_getitem_dimension(self, Worksheet):
        ws = Worksheet(Workbook())
        cells = ws["A1:E2"]
        expected = (
            (ws['A1'], ws['B1'], ws['C1'], ws['D1'], ws['E1']),
            (ws['A2'], ws['B2'], ws['C2'], ws['D2'], ws['E2']),
        )
        assert tuple(cells) == expected

Comments (3)

  1. CharlieC

    start and stop are slice attributes. There is no need for sanity checking.

    The supplied unit tests do not add anything.

  2. Peter Pul reporter

    I understand that start and stop are attributes of slice, but before I understood "A1":"E2" works I tried 1:5 and :, which all make the __getitem__ crash.

    And I don't understand why the tests aren't added. Altough the unit test doesn't really add something, both should be added to test all possible behaviour on __getitem__.

  3. CharlieC
    ws[1:5]
    ((<Cell Sheet.A1>,),
     (<Cell Sheet.A2>,),
     (<Cell Sheet.A3>,),
     (<Cell Sheet.A4>,),
     (<Cell Sheet.A5>,))
    

    In 2.4 slices and range references are equivalent (I'm not going to backport this) so this could be tested for ws['A1':'E2'] == ws['A1:E2']. However, for an empty workbook both will return an empty tuple (and your tests would fail in 2.4).

    The same is true for an empty slice ws[:] == ws[':']. This should raise an exception in both cases (it's a weird thing to ask for, IMO), though the choice of exception is open to debate – I would like to retire InsufficientCoordinatesException as an IndexError (preferable if worksheets are to be thought of as lists or matrices) or KeyError are sufficient.

    Use py.test --cov path/tomodule --cov-report term-missing path/totests to see whether more tests are required.

  4. Log in to comment