vs-1997 - #1
Conversation
rklyne
left a comment
rklyne
left a comment
There was a problem hiding this comment.
Broadly good.
Some bits that need adjusting to make the code more robust.
|
|
||
| import sys | ||
|
|
||
| import openpyxl |
There was a problem hiding this comment.
Is this being added back because the removal of this code is not complete yet?
There was a problem hiding this comment.
this import was moved from line 14
|
|
||
|
|
||
| def dset_sheet(dataset, ws, freeze_panes=True): | ||
| def dset_sheet(dataset, ws, cell_formats): |
There was a problem hiding this comment.
We would probably need to add freeze_panes back in before this would be accepted upstream
There was a problem hiding this comment.
I can restore it, but, currently, this parameter isn't used in the function
| :param column_name: column name, str | ||
| :return: column index, int | ||
| """ | ||
| if column_name: |
| :param aligment: text position in the cell, str | ||
| :return: dict | ||
| """ | ||
| return { |
There was a problem hiding this comment.
Future improvement - make this a Format() object to support future improvements without changing the API.
There was a problem hiding this comment.
Cool, let's discuss it internally
| return { | ||
| 'first_row': first_row, | ||
| 'first_col': first_col, | ||
| 'last_row': last_row, |
There was a problem hiding this comment.
Do we really have to store last_row and last_col ?
My concern here is that those change as the sheet grows.
There was a problem hiding this comment.
yes, we need those values in data_validation and condition_format methods:
https://xlsxwriter.readthedocs.io/worksheet.html#data_validation
https://xlsxwriter.readthedocs.io/worksheet.html#conditional_format
because we are working with the range of the cells
| for condition in conditions: | ||
| cell_format = {'bg_color': condition.get('color', None)} | ||
| value = '"{}"'.format(condition['value']) if isinstance(condition['value'], str) else condition['value'] | ||
| self._set_conditional_format(1, column_position, len(self._package(dicts=False)) - 1, |
There was a problem hiding this comment.
We should avoid generating _package here - it's an expensive operation.
self.height should be enough.
That said, it might be better to avoid height entirely.
What happens if you add more columns after this conditional formatting?
I think they don't get the conditional formatting applied.
It would be more robust to record the column only.
There was a problem hiding this comment.
ok, I will avoid it, thanks for the hint.
I think we will add formats after that when all needed records will be created
|
Looks good to me. Will await Ronan's view on his comments. A 'nice touch' might be to set the column widths. I am now thinking about how to incorporate what you have added into the Generic Reporting that uses Tablib. The new XLSX format option is easy; it is the other features like headers and footers and formats. |
meshy
left a comment
meshy
left a comment
There was a problem hiding this comment.
While these changes may be valuable additions to the library, I believe that maintaining our own fork of tablib is unwise. The upstream fork is maintained, so we should be opening PRs against it, rather than here. This has several benefits:
- We will continue getting updates, including security fixes.
- We will not have to duplicate upstream work to support new versions of Python (etc).
- We can share these our contributions with the community.
There are a couple of minor things we could do to make this more attractive to upstream. In particular, we should:
- Open an issue to ask them if they want this new functionality. If they do not feel that it is a worthwhile addition to the project, they may be able to suggest an alternative, complementary, implementation (eg: a plugin).
- Make the commit messages adhere to a standard format. At the moment, they contain our ticket references, some of the messages have been repeated, and the history could do with cleaning up.
4 participants
Unittests will be added in separate ticket.