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

Allow users to pass vega_options #10

Merged
merged 11 commits into from
Oct 7, 2024
Merged

Conversation

ddanieltan
Copy link
Contributor

#8

This PR adds the vega_options parameter to altair2fasthtml which gives users a choice to pass additional vegaEmbed options. This parameter is optional, and defaults to the existing setting of actions:False

@ddanieltan
Copy link
Contributor Author

Couple of notes:

  • I opted to add a new parameter vega_options instead of the catch all **kwargs discussed in the issue, because based on our discussion in Responsive sized charts #9, we were planning to add params like full_width to altair2fasthtml and I feel that it would be cleaner to have an explicit param for vega_options vs. adding parsing logic with a catch all **kwargs implementation
  • The unit test I added feels kinda janky to me because I'm extracting the __str__ of the returned Div(). I asked the FastHTML discord if there's a better way to write unit tests for FT() objects and might come back to improve the unit test if I hear back
  • I use an autoformatter (Ruff with vanilla settings) which reformatted the files I touched. Hope you don't mind, happy to revert formatting if you prefer me to

Following advice from FastHTML discord
Copy link
Owner

@koaning koaning left a comment

Choose a reason for hiding this comment

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

Left a minor comment on testing, but looks great!

return altair2fasthtml(chart)


def test_vega_options_passed_to_vegaEmbed():
Copy link
Owner

Choose a reason for hiding this comment

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

Bit of a nit, but maybe parametrize this test so we loop over all the options?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to think about how to parameterize the 2 tests (and also asked Claude for some help) and the resulting solution felt less readable to me. I think it's because test_no_err calls altair2fasthtml() with a single argument and does not use an assert, whereas test_vega_options_passed_to_vegaEmbed calls altair2fasthtml() with multiple arguments and does use an assert.

So when trying to parameterize both of these tests into a single function, I ended up having to add what looks to be unnecessary boolean checks.

I opted instead to just put the repeated lines for generating the sample chart into a fixture so that eliminates the repetition. Let me know if you think this is an okay approach, I could be overthinking it.

Copy link
Owner

Choose a reason for hiding this comment

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

I might advise to turn Claude off sometimes, it does give bad advise now and again.

That said, I was merely thinking along these lines:
https://github.com/koaning/fh-altair/pull/10/files#r1787928395

I agree that they are just boolean checks, but they do serve as a wide smoke test. It runs fast, but can tell us if our expectation is broken quite quickly.

It is a detail though, so I can also just do this myself if that is preferable. It is a preference that lives in my mind, but it is totally fine if not everyone has the same idea on how useful these kinds of tests are.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good advice! Thank you for the example. I think I misunderstood your original suggestion to somehow use parameterize to combine both test_no_err and test_vega_options_passed_to_vegaEmbed into a single test.

Instead it's clear to me now that you were suggesting to use parameterize to go pass all combinations of inputs solely to test_vega_options_passed_to_vegaEmbed, that's straightforward and I can make that change.

README.md Outdated Show resolved Hide resolved
return altair2fasthtml(sample_chart)


def test_vega_options_passed_to_vegaEmbed(sample_chart):
Copy link
Owner

Choose a reason for hiding this comment

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

I was initially thinking something like this:

Suggested change
def test_vega_options_passed_to_vegaEmbed(sample_chart):
@pytest.mark.parametrize("renderer,actions", [("svg", True), ("svg", False)])
def test_vega_options_passed_to_vegaEmbed(sample_chart, renderer, actions):

I agree that it isn't the most descriptive test, but looping over the settings like this should at least ensure that for the settings that we expect nothing is breaking. I would also usually use itertools.product to generate all the combinations of settings.

@koaning koaning merged commit 61fe723 into koaning:main Oct 7, 2024
1 check passed
@ddanieltan ddanieltan deleted the vega-embed-kwargs branch October 8, 2024 07:16
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.

2 participants