0

This code returns a list of widgets from a spreadsheet range that has another range indicating it is enabled for processing. The code works but it is poorly implemented, especially in terms of proper use of zip and the layout.

I'm fairly sure that zip and ws.iter_cols should be part of the answer but not on the best way to use them. The only significant line is the List comprehension under the Row/column code comment. So, how should it be done?

The code needs to run on various ranges but was initially hard-coded for development. In openpyxl row/column processing is more efficient than coordinate ranges and coordinate range functionality is progressively being depreciated. (The coordinate code is only used for development and will be deleted.)

PEP8 is giving continuation line under-indented for visual indent warnings.

from __future__ import print_function
from openpyxl import Workbook
from openpyxl.utils import get_column_letter
import openpyxl


def do_widgets(ws, col0, col1):
    """ Process each widget. """
    NO_WDGTS = 5
    ENABLE_START_ROW = 6
    ENABLE_END_ROW = ENABLE_START_ROW + NO_WDGTS - 1
    WDGT_START_ROW = 1
    WDGT_END_ROW = WDGT_START_ROW + NO_WDGTS - 1

    # Coordinate code - delete whole block before implementation
    col_letter0 = get_column_letter(col0)
    col_letter1 = get_column_letter(col1)
    wdgos_range = [(col_letter0 + str(ENABLE_START_ROW) + ":" +
                    col_letter0 + str(ENABLE_END_ROW))]
    wdgos_range.append(col_letter1 + str(WDGT_START_ROW) + ":" +
                       col_letter1 + str(WDGT_END_ROW))
    # Delete
    # wdgos = [cell[1][0].value for cell in zip(ws['B6:B10'], ws['C1:C5'])
    #                                          if cell[0][0].value]
    wdgos = [cell[1][0].value for cell in zip(ws[wdgos_range[0]], ws[wdgos_range[1]])
                                             if cell[0][0].value]

    # Row/column code
    wdgts = [cell[1] for cell in zip([cell.value for col
                     in ws.iter_cols(min_row=ENABLE_START_ROW, min_col=col0,
                                     max_row=ENABLE_END_ROW, max_col=col0)
                     for cell in col],
                     [cell.value for col
                     in ws.iter_cols(min_row=WDGT_START_ROW, min_col=col1,
                                     max_row=WDGT_END_ROW, max_col=col1)
                     for cell in col]) if cell[0]]

    # Use list of enabled widgets
    print("Enable range [0], Widget range [1]")
    print(wdgos_range)
    for wdgo in wdgos:
            print(wdgo)
    print()
    print(wdgts, '\n')


wb = Workbook()
ws = wb.active

rows = [
    ['Number', 'Batch 1', 'Batch 2'],
    [2, 40, 30],
    [3, 40, 25],
    [4, 50, 30],
    [5, 30, 10],
    [0, 0, 0],
    [0, 1, 0],
    [1, 1, 0],
    [0, 0, 1],
    [0, 1, 0],
]

for row in rows:
    ws.append(row)
# Worksheet, enable column, widget column
do_widgets(ws, 2, 3)

Debug output:

Enable range [0], Widget range [1]
['B6:B10', 'C1:C5']
30
25
10

[30, 25, 10]
flywire
  • 1,155
  • 1
  • 14
  • 38

1 Answers1

1

perhaps you could simplify it a bit as follows:

#define cell "extractor" from given position
def get_cells(row, col):
    for col in ws.iter_cols(min_row = row, min_col = col, max_row = row + NO_WDGTS - 1, max_col = col):
        for cell in col:
            yield cell.value

#use itertools.izip to zip the generators in order to process
#the elements one by one instead of generating them all at once 
wdgts = [cell[1] for cell in itertools.izip(
            get_cells(ENABLE_START_ROW, col0),
            get_cells(WDGT_START_ROW, col1)) if cell[0]]
ewcz
  • 12,819
  • 1
  • 25
  • 47
  • I don't understand the point of `process the elements one by one instead of generating them all at once` - changed code slightly and zipped the lot. Been working well. – flywire Jun 01 '18 at 11:22
  • @flywire the [advantage](https://stackoverflow.com/a/4989890/5351549) of `izip` is that (in contrast to `zip`) it doesn't need to extract all the cells first in order to iterate over them. It consumes the `get_cells` generators "on-the-fly"... But unless you have thousands of cells, the difference/impact will be most likely negligible. – ewcz Jun 01 '18 at 14:03