-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
222 gridlines on selected sheets #228
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left one comment about parameter options but this otherwise looks good 😃
Please can you update the tests to also check the gridlines, or let me know if there are any problems writing tests for this
I think a future enhancement would be for users to be able to specify which sheets should have gridlines using a dictionary but I want to get this approach merged in first and leave that for the future |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making those changes. Just to check - should the numbers be 0-2 or 1-3?
gptables/core/api.py
Outdated
if cover_gridlines: | ||
ws = wb.add_worksheet(cover.cover_label, gridlines=gridlines) | ||
else: | ||
ws = wb.add_worksheet(cover.cover_label, gridlines=2) | ||
ws = wb.add_worksheet(cover.cover_label, gridlines="hide_all") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the if-else is necessary if we specify a default value in the function definition? Does this work if you just have ws = wb.add_worksheet(cover.cover_label, gridlines=gridlines)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The if-else is for when you want gridlines on the workbook but not on the cover sheet specifically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
API test now passing and the expected workbook looks reasonable to me, so happy for this to get merged in now
Added a parameter in write/produce_workbook to show or hide gridlines on the worksheets and another parameter to choose whether to include the gridlines on the cover worksheet.