-
Notifications
You must be signed in to change notification settings - Fork 17
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
Filterx format csv #132
Filterx format csv #132
Conversation
0bc0e91
to
0830d8a
Compare
|
||
self->super.super.eval = _eval; | ||
self->super.super.free_fn = _free; | ||
self->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.
the default delimiter for the parse_csv() function is a space, so I think we should match that here.
{ | ||
guint64 size; | ||
if (!filterx_object_len(csv_data, &size)) | ||
return FALSE; |
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.
we should return NULL here.
FilterXObject *csv_data = filterx_expr_eval_typed(self->input); | ||
if (!csv_data) | ||
{ | ||
filterx_eval_push_error("Failed to evaluate input. " FILTERX_FUNC_FORMAT_CSV_USAGE, s, NULL); |
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 we don't need to push the error here as filterx_expr_eval() would set that already.
success = _append_to_buffer(NULL, elt, user_data); | ||
filterx_object_unref(elt); | ||
} | ||
} |
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.
[optional] I'd extract the specializations to separate functions to make this a bit easier to follow.
filterx_object_unref(col); | ||
filterx_object_unref(elt); | ||
} | ||
} |
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.
we should handle if columns is not a list (return NULL with an 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.
now, I can see that in that case, success is FALSE, so that's what we would do.
I think extracting the type specific specializations would have avoided my misunderstanding.
else | ||
{ | ||
filterx_eval_push_error("input must be a dict or list. " FILTERX_FUNC_FORMAT_CSV_USAGE, s, csv_data); | ||
filterx_object_unref(csv_data); |
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.
we do the same in the epilogue part, as sucess is FALSE in this case.
g_string_append_c(buffer, '"'); | ||
append_unsafe_utf8_as_escaped_binary(buffer, value_buffer->str, value_buffer->len, "\""); | ||
g_string_append_c(buffer, '"'); | ||
|
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 is kind of the "dialect" parameter we already support on the parse_csv() side. the dialect would determine what kind of quotation we produce on the output side.
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 is looking good, I had a few comments, some of which would be worth addressing before merging this.
@bshifter Just an idea: |
0830d8a
to
1dd210c
Compare
After discussing this with @bshifter , it seems better to let this approach go. |
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 is only a partial review. I will continue the review tomorrow.
{ | ||
guint64 size; | ||
if (!filterx_object_len(cols, &size)) | ||
return FALSE; |
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 leaks cols
.
} | ||
|
||
static gboolean | ||
_handle_dict_input(FilterXFunctionFormatCSV *self, FilterXObject *csv_data, GString *formatted) |
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.
[optional]
Usually we functions are written with early returns in axosyslog, as it is more readable. Can you reorganize this function? Thanks!
Signed-off-by: shifter <[email protected]>
Signed-off-by: shifter <[email protected]>
Signed-off-by: shifter <[email protected]>
1dd210c
to
23908cb
Compare
add csv formatter function to filterx, helps re-format csv data parsed by parse_csv previously
examples: