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

feat: add save dynamic csv #5130

Merged
merged 3 commits into from
Jan 11, 2024
Merged

Conversation

klDen
Copy link
Contributor

@klDen klDen commented Jan 3, 2024

Description

Add save dynamic CSV files.

Copy link

codecov bot commented Jan 3, 2024

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (1671db7) 63.34% compared to head (6baf264) 63.49%.
Report is 12 commits behind head on main.

Files Patch % Lines
...o/extra/csv/dynamic/syntax/SCollectionSyntax.scala 87.50% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5130      +/-   ##
==========================================
+ Coverage   63.34%   63.49%   +0.14%     
==========================================
  Files         291      293       +2     
  Lines       10840    10844       +4     
  Branches      753      752       -1     
==========================================
+ Hits         6867     6885      +18     
+ Misses       3973     3959      -14     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

import com.spotify.scio.values.SCollection
import kantan.csv._
import kantan.codecs.compat._
Copy link
Contributor

Choose a reason for hiding this comment

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

IntelliJ probably tells this import is unused for 2.13, but it is required for +compile with 2.12

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. Re-adding it

* }}}
*/
package object csv extends AllSyntax {
final private[scio] class CsvSink[T: HeaderEncoder](csvConfig: CsvConfiguration)
Copy link
Contributor

Choose a reason for hiding this comment

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

Defining classes in package object has been discouraged. This creates the following warning:

it is not recommended to define classes/objects inside of package objects.
If possible, define class CsvSink in package csv instead.

It would be nicer to move this to another compile unit inside the csv package.

Copy link
Contributor Author

@klDen klDen Jan 5, 2024

Choose a reason for hiding this comment

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

Got it. I'll move CsvSink in a dedicated file under scio.extra.csv

Is there a plan to migrate other classes that are defined in package objects? e.g. https://github.com/klDen/scio/blob/d97f0387393315d89fd0bbd6684306bd5f37a376/scio-parquet/src/main/scala/com/spotify/scio/parquet/package.scala

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, this is a work in progress: #5127

@klDen klDen force-pushed the feat/add-dynamic-csv-rw branch 2 times, most recently from ed7573f to c027145 Compare January 5, 2024 19:53
@klDen klDen requested a review from RustedBones January 5, 2024 19:54
@klDen klDen force-pushed the feat/add-dynamic-csv-rw branch 2 times, most recently from 308aaac to 91bb15c Compare January 5, 2024 20:42
Copy link
Contributor

@RustedBones RustedBones left a comment

Choose a reason for hiding this comment

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

LGTM. I took the freedom to refactor the test a bit.
Thanks for the contribution!

Copy link
Contributor

@clairemcginty clairemcginty left a comment

Choose a reason for hiding this comment

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

lgtm! Thanks for contributing+adding the unit tests!

@RustedBones RustedBones merged commit 280518c into spotify:main Jan 11, 2024
9 of 10 checks passed
@klDen klDen deleted the feat/add-dynamic-csv-rw branch January 11, 2024 16:59
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.

3 participants