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

Fix error from incorrectly passing nanoplot options to args in _generate_nanoplot() #258

Merged
merged 1 commit into from
Apr 1, 2024

Conversation

rich-iannone
Copy link
Member

@rich-iannone rich-iannone commented Mar 31, 2024

In a previous PR, a call to _generate_nanoplot() was simplified to use ** to pass through args:

**options_plots,

However, the argument names in three cases don't match. These are:

  • line_type != data_line_type
  • show_ref_line != show_reference_line
  • show_ref_area != show_reference_area

The solution here is to expand the shorter arg names to their longer counterparts. This doesn't break the GT API since the longer names are what the user has access to within nanoplot_options().

With this change, examples in https://posit-dev.github.io/great-tables/blog/introduction-0.4.0/ should no longer be broken.

@codecov-commenter
Copy link

codecov-commenter commented Mar 31, 2024

Codecov Report

Attention: Patch coverage is 82.35294% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 75.26%. Comparing base (cd618a4) to head (ff69cb5).

Files Patch % Lines
great_tables/_utils_nanoplots.py 82.35% 3 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #258   +/-   ##
=======================================
  Coverage   75.26%   75.26%           
=======================================
  Files          41       41           
  Lines        4180     4180           
=======================================
  Hits         3146     3146           
  Misses       1034     1034           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@machow machow left a comment

Choose a reason for hiding this comment

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

Shoot--thanks for fixing, and sorry for sacking all nanoplots with my clean up 😓. I'll merge now, since it's easy to confirm in the generated docs it's working, but let's be sure to add some basic fmt_nanoplot tests that, so the tests flag these situations

@machow machow merged commit af6bd14 into main Apr 1, 2024
7 checks passed
@rich-iannone rich-iannone deleted the fix-nanoplots-args-naming branch April 1, 2024 02:41
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.

None yet

3 participants