-
Notifications
You must be signed in to change notification settings - Fork 3
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: Include CSV headers #30
Conversation
csv/write.go
Outdated
@@ -7,8 +7,17 @@ import ( | |||
"github.com/cloudquery/plugin-sdk/schema" | |||
) | |||
|
|||
func WriteTableBatch(w io.Writer, _ *schema.Table, resources [][]any) error { | |||
func WriteTableBatch(w io.Writer, table *schema.Table, resources [][]any, headers bool) error { |
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 can't come up with any other way of injecting this conditional headers
argument... Suggestions would be appreciated, or I can just add a nolint to 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 think they basically want you to split it into different functions, e.g. WriteTableBatch
and WriteTableBatchNoHeaders
. It might be better to force users to think about whether they want headers or not though, so I'm fine with adding nolint
for this case
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.
So I will just add the nolint
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 don't see how that can be possible, unless you still had a private function with the header boolean? In that case you could rather have WriteTableBatch
write the headers first, then call WriteTableBatchNoHeaders
to do the rest
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.
(Or something to that effect - probably won't work just like that) In any case, nolint is fine.
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.
Thinking about this some more... the function WriteTableBatch(w io.Writer, table *schema.Table, resources [][]any)
is a standard interface used throughout many destinations, and the plugin sdk... Maybe we need a more generic way of passing in arguments...
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.
So maybe it is better to add an entirely new function than it is to break that function signature...
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.
Yeah, though I think the filetypes don't need to conform to that interface. (CC @yevgenypats because he'll probably know better)
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.
A generic way of passing arguments sounds good though, as we'll hopefully be adding support for more formats (like parquet) in the not-too-distant future as well.
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 feel the current approach in this PR is the right one as there is no generic interface needed here and the caller need to know wheater they want to use headers or not.
Un related to that if we already add options to that table I would make this just a bit nicer with ...Option so we can extend this later without breaking the interface (For example, if we would want an option of changing a delimiter ).
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.
looks good but seems the interface needs a bit of change because now we want to add options to write
and read
which means we actually want a NewClient(...Option) *Client
which will have WriteTableBatch
and Read
methods.
and one option for nowOptionWithHeaders
csv/write.go
Outdated
@@ -7,8 +7,17 @@ import ( | |||
"github.com/cloudquery/plugin-sdk/schema" | |||
) | |||
|
|||
func WriteTableBatch(w io.Writer, _ *schema.Table, resources [][]any) error { | |||
func WriteTableBatch(w io.Writer, table *schema.Table, resources [][]any, headers bool) error { |
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 feel the current approach in this PR is the right one as there is no generic interface needed here and the caller need to know wheater they want to use headers or not.
Un related to that if we already add options to that table I would make this just a bit nicer with ...Option so we can extend this later without breaking the interface (For example, if we would want an option of changing a delimiter ).
csv/read.go
Outdated
@@ -23,9 +23,6 @@ func Read(f io.Reader, table *schema.Table, sourceName string, res chan<- []any) | |||
} | |||
return err | |||
} | |||
if record[sourceNameIndex] != sourceName { |
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 this is removed? also, shouldn't the no header propogate 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.
This was removed because when there are headers the row does not get read...
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.
Looks good. few comments.
…iletypes into include-csv-headers
🤖 I have created a release *beep* *boop* --- ## [1.1.0](v1.0.6...v1.1.0) (2023-01-18) ### Features * Include CSV headers ([#30](#30)) ([9ab6df8](9ab6df8)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Summary
Adds ability to conditionally include headers for CSV files