Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Allow users to pass vega_options #10
Changes from 6 commits
6bca08b
0a9750e
2d02bd8
2529cc0
022e952
4779e09
50a4c50
52f1fdb
8a5552a
1541f6c
ca06198
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Bit of a nit, but maybe parametrize this test so we loop over all the options?
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 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
callsaltair2fasthtml()
with a single argument and does not use an assert, whereastest_vega_options_passed_to_vegaEmbed
callsaltair2fasthtml()
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.
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 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.
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.
Good advice! Thank you for the example. I think I misunderstood your original suggestion to somehow use parameterize to combine both
test_no_err
andtest_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.