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
Fixturize JSON tests #31191
Fixturize JSON tests #31191
Changes from all commits
730ed79
1a96b4f
dfebc6b
e793582
d507eb4
224dd5a
0056824
bb808df
9f57295
65de92a
2b78450
4e80178
083bfdb
e60430b
dc2d93e
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.
did this get resolved?
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.
this looks really similar to test_series_roundtrip_empty and test_series_roundtrip_object. would it make sense to something like
any_series
fixture for this?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.
Yea I think a great idea. I’m thinking worth doing after breaking out and setting up test_roundtrip.py with other parametrization unless a blocker here
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.
sounds good. This LGTM
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.
why change this? doesn't the series type depends on the _index_factory fixture.
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.
Trying to avoid naming conflicts with the top level
empty_series
fixture, which I think is more general; would they not conflict if named the same?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.
see #24886, @jbrockmendel removed empty series fixture since it is a singleton and inlined instead. adding a top-level empty_series fixture is inconsistent with the series tests.
The fixture here is a composable fixture and more generic and IMO should not be renamed.
Does the fixture added in this PR need to be in the top level conftest or could it be just added to pandas/tests/io/json/test_pandas.py as a module level fixture?
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.
IIUC the all_ts decorator is running this test for date_range, period_range and timedelta_range, not just dti