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

Move dependencies to devDependencies #3

Merged
merged 4 commits into from
Oct 7, 2024
Merged

Conversation

wesbiggs
Copy link
Member

@wesbiggs wesbiggs commented Oct 4, 2024

Don't bring in avsc or @dsnp/parquetjs as core dependencies (still used for tests).

Work in progress, parquet.ts needs work

…ed for tests).

Work in progress, parquet.ts needs work
@wesbiggs wesbiggs requested a review from wilwade October 4, 2024 17:43
@wesbiggs wesbiggs marked this pull request as draft October 7, 2024 02:40
@wilwade wilwade marked this pull request as ready for review October 7, 2024 14:34
@@ -72,6 +72,7 @@ ParquetSchema {
"compression": "GZIP",
"dLevelMax": 0,
"encoding": "PLAIN",
"logicalType": undefined,
Copy link
Member

Choose a reason for hiding this comment

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

The latest version of Parquetjs also has this field


const [parquetSchema, writerOptions] = parquet.fromDSNPSchema(descriptorForAnnouncementType(AnnouncementType.Broadcast).parquetSchema);
const writer = await ParquetWriter.openFile(parquetSchema, "./file.parquet", writerOptions);
const writer = await ParquetWriter.openFile(new ParquetSchema(parquetSchema), "./file.parquet", writerOptions);
Copy link
Member

Choose a reason for hiding this comment

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

This is the change we should highlight in the changelog. Effectively instead of constructing this class object in here, we require devs to do it outside. Not a bad tradeoff for dropping the dependency.

Copy link
Member

@wilwade wilwade left a comment

Choose a reason for hiding this comment

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

Approved, but I wrote some of it.

@wilwade wilwade merged commit 53f95c8 into main Oct 7, 2024
1 check passed
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