You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by "gnwgnw (via GitHub)" <gi...@apache.org> on 2025/02/20 07:54:45 UTC
[PR] Alternative implementation of Stringer interface [thrift]
gnwgnw opened a new pull request, #3106:
URL: https://github.com/apache/thrift/pull/3106
<!-- Explain the changes in the pull request below: -->
JSON marshaling allows to log a value of optional fields instead of addresses.
<!-- We recommend you review the checklist/tips before submitting a pull request. -->
- [ ] Did you create an [Apache Jira](https://issues.apache.org/jira/projects/THRIFT/issues/) ticket? ([Request account here](https://selfserve.apache.org/jira-account.html), not required for trivial changes)
- [ ] If a ticket exists: Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"?
- [x] Did you squash your changes to a single commit? (not required, but preferred)
- [x] Did you do your best to avoid breaking changes? If one was needed, did you label the Jira ticket with "Breaking-Change"?
- [ ] If your change does not involve any code, include `[skip ci]` anywhere in the commit message to free up build resources.
<!--
The Contributing Guide at:
https://github.com/apache/thrift/blob/master/CONTRIBUTING.md
has more details and tips for committing properly.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Alternative implementation of Stringer interface [thrift]
Posted by "Jens-G (via GitHub)" <gi...@apache.org>.
Jens-G closed pull request #3106: Alternative implementation of Stringer interface
URL: https://github.com/apache/thrift/pull/3106
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Alternative implementation of Stringer interface [thrift]
Posted by "Jens-G (via GitHub)" <gi...@apache.org>.
Jens-G commented on PR #3106:
URL: https://github.com/apache/thrift/pull/3106#issuecomment-3638967023
Status still unclear, no activity anymore. Feel free to send a new PR
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Alternative implementation of Stringer interface [thrift]
Posted by "Jens-G (via GitHub)" <gi...@apache.org>.
Jens-G commented on PR #3106:
URL: https://github.com/apache/thrift/pull/3106#issuecomment-3567971213
What's the status of this one?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Alternative implementation of Stringer interface [thrift]
Posted by "fishy (via GitHub)" <gi...@apache.org>.
fishy commented on code in PR #3106:
URL: https://github.com/apache/thrift/pull/3106#discussion_r1977848158
##########
compiler/cpp/src/thrift/generate/t_go_generator.cc:
##########
@@ -1425,8 +1425,15 @@ void t_go_generator::generate_go_struct_definition(ostream& out,
out << indent() << "return \"<nil>\"" << '\n';
indent_down();
out << indent() << "}" << '\n';
- out << indent() << "return fmt.Sprintf(\"" << escape_string(tstruct_name) << "(%+v)\", *p)"
- << '\n';
+ if (stringer_mode_ == STRINGER_MODE_FMT) {
+ out << indent() << "return fmt.Sprintf(\"" << escape_string(tstruct_name) << "(%+v)\", *p)"
+ << '\n';
+ } else if (stringer_mode_ == STRINGER_MODE_JSON) {
+ out << indent() << "v, _ := json.Marshal(p)" << '\n';
Review Comment:
in general, we don't want to drop error returns from generated go code. I would prefer to fallback to the old behavior if we hit an error when doing json marshaling here.
additionally, this is not the most efficient way to do json marshaling, especially when the struct is large, as it requires first grow the `[]byte` used in `json.Marshal` until it can fit the whole string, then it need to be allocated again when you convert that to `string`. A more efficient way to do it would be using `json.NewEncoder` with `strings.Builder`, which will avoid the second allocation, but you need to strip the final `\n` it generates.
we actually already implemented it in [`SlogTStructWrapper`](https://pkg.go.dev/github.com/apache/thrift/lib/go/thrift#SlogTStructWrapper), you can check https://github.com/apache/thrift/blob/v0.21.0/lib/go/thrift/slog.go#L46 for the implementation. that implementation will only do the json encoding when the struct is logged via slog. the consideration is that json encoding is much more heavier than just `fmt.Sprintf`, so we only do that when needed (e.g. when you need to log it), instead of everywhere the stringer implementation is used (an example is that for compiler generated code for an exception, its `Error` implementation also uses stringer implementation under the hood). we definitely do not want to change default stringer implementation to json as that would have large performance implications. your approach of adding a compiler arg is most likely a good compromise, but please take a look at `SlogTStructWrapper`, and maybe that's already enough for your use-case :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Alternative implementation of Stringer interface [thrift]
Posted by "fishy (via GitHub)" <gi...@apache.org>.
fishy commented on PR #3106:
URL: https://github.com/apache/thrift/pull/3106#issuecomment-2694992147
also cc @dcelasun
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org