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

Restructure test suite and add file IO roundtrip tests #90

Merged
merged 3 commits into from
Feb 26, 2024

Conversation

nicholasjng
Copy link
Collaborator

Scraps multi-dir discovery, improves code readability, and shortens a
few filenames in the benchmark suite.

Most of these tests passed out of the box, except for CSV, which incurred
some trouble with flattened contexts and quotechars.

As a consequence, the CSV reader and writer get a small block of control
flow, which JSON-loads the context and _contextkeys attributes if present,
since Python-native CSV coerces everything to string. This requires some
quotechar manipulation, and changing over the _contextkeys attribute
to a list of strings instead of a tuple, since json.load cannot deal with
round parentheses.

Scraps multi-dir discovery, improves code readability, and shortens a
few filenames in the benchmark suite.
@nicholasjng nicholasjng added the tests Related to tests of the package. label Feb 26, 2024
@nicholasjng nicholasjng self-assigned this Feb 26, 2024
Most of these tests passed out of the box, except for CSV, which incurred
some trouble with flattened contexts and quotechars.

As a consequence, the CSV reader and writer get a small block of control
flow, which JSON-loads the context and _contextkeys attributes if present,
since Python-native CSV coerces everything to string. This requires some
quotechar manipulation, and changing over the `_contextkeys` attribute
to a list of strings instead of a tuple, since json.load cannot deal with
round parentheses.
@nicholasjng
Copy link
Collaborator Author

For reviewers:

Is it worth inferring the schema from the CSV records? Upside is type safety, downside is more (and potentially difficult) implementation necessary. Note that a user can plug in his pd.read_csv() easily via register_driver_implementation("csv", ...).

@nicholasjng nicholasjng merged commit 0d47d7b into main Feb 26, 2024
5 checks passed
@nicholasjng nicholasjng deleted the restructure-tests branch March 14, 2024 08:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests Related to tests of the package.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant