Skip to content
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

New examples #229

Merged
merged 29 commits into from
Jul 3, 2024
Merged

New examples #229

merged 29 commits into from
Jul 3, 2024

Conversation

danmilne1
Copy link
Contributor

No description provided.

@danmilne1 danmilne1 requested a review from rowanhemsi October 16, 2023 15:18
@ellie-o ellie-o linked an issue Jun 27, 2024 that may be closed by this pull request
Copy link
Collaborator

@rowanhemsi rowanhemsi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really great job, thank you for making these changes. Left a couple of comments but I think these examples should replace the old ones.
On that note, is there any functionality in the old examples that isn't covered in the penguins examples? The only thing I can think of is the reticulate example - would you be happy to make a penguins equivalent?

"Just another subtitle"
]
penguins_scope = "Penguins"
penguins_source = "Source: Office for Penguin Statistics"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe actual source for this dataset?

-----------------

This example demonstrates use of the ``gptables.Cover`` class to create a cover page. This example also
demonstrates the usage of the ``index_columns`` argument and how to create a workbook with multiple sheets.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this example should focus on using more than one sheet and not talk about indexing?

iris_summary = iris_summary.pivot_table(
index=["Iris feature", "Average"], columns="index", values="value"
).reset_index()
#Any data processing could go here as long as you end with a Pandas dataframe that you want to write in a spreadsheet
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much better to replace the wrangling with a comment, thank you for doing this

Comment on lines 94 to 101
#To format cells using the set_row or set_column functions we must use a workbook to create a format object
italic_format=wb.add_format({"italic":True})
ws.set_column(2,3,10,italic_format) #Sets the width of the third and fourth column and makes them italic

#Note that the first two arguments of set_column are the first and last columns (inclusive) you want to format as opposed
#to set_row which only affects a single row at a time (the first argument).

# Finally use the close method to save the output
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really helpful addition

Comment on lines 42 to 50
# or use kwargs to pass these to the appropriate parameters
# kwargs = {
# "table_name": penguins_table_name,
# "title": penguins_title,
# "subtitles": penguins_subtitles,
# "scope": penguins_scope,
# "source": penguins_source,
# }
#penguins_table = gpt.GPTable(table=penguins_data, **kwargs) would also be valid
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe cut this for the minimal example?

Where you wish to provide no metadata in required parameters, use ``None``.

The theme parameter must take either a directory or a yaml file in the ``gptables.write_workbook`` function.
The yaml file used in this example can be found in the themes folder as ''penguins_test_theme.yaml''.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the theme used for testing? If not, let's swap test for example or just remove it

@rowanhemsi rowanhemsi changed the base branch from main to dev June 28, 2024 11:00
@rowanhemsi rowanhemsi self-requested a review June 28, 2024 14:39
Copy link
Collaborator

@rowanhemsi rowanhemsi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general looks great. I think it's worth keeping the custom theme example but otherwise I'm good with these changes 👍

:language: R

R users may also be interested in the `a11ytables <https://co-analysis.github.io/a11ytables/>`_
However we recommend use of the `a11ytables <https://co-analysis.github.io/a11ytables/>`_
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a good recommendation

@@ -0,0 +1,58 @@
"""
Penguins - Cover Page
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy paste error?

Penguins - Cover Page
-----------------

This example demonstrates how to create a workbook with multiple sheets.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding a comment to explain that a table of contents will be autogenerated

@@ -9,8 +9,7 @@
Where you wish to provide no metadata in required parameters, use ``None``.

The theme parameter must take either a directory or a yaml file in the ``gptables.write_workbook`` function.
The yaml file used in this example can be found in the themes folder as ''penguins_test_theme.yaml''.
The personalised theme removes any bold or italics from the table.
The yaml file used in this example can be found in the themes folder as ''gptheme.yaml''.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is worth keeping a separate theme for this example, because I'd like to avoid people editing the default theme (both for good practice and because it's actually protected and might confuse people)

Copy link
Collaborator

@rowanhemsi rowanhemsi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making these final changes, this looks great

@danmilne1 danmilne1 merged commit 7bac368 into dev Jul 3, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Simplify basic example(s)
2 participants