You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by "Bryan Duxbury (JIRA)" <ji...@apache.org> on 2008/08/20 22:54:46 UTC
[jira] Created: (THRIFT-119) Improved toString for Java Structs
Improved toString for Java Structs
----------------------------------
Key: THRIFT-119
URL: https://issues.apache.org/jira/browse/THRIFT-119
Project: Thrift
Issue Type: Sub-task
Reporter: Bryan Duxbury
Assignee: Bryan Duxbury
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-119) Improved toString for Java Structs
Posted by "Mark Slee (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-119?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12625494#action_12625494 ]
Mark Slee commented on THRIFT-119:
----------------------------------
+ indent(out) << "if ((skipOptionalNulls && __isset." << (*f_iter)->get_name() << " != false) || !skipOptionalNulls) {" << endl;
This condition can be written more succinctly as:
+ indent(out) << "if (!skipOptionalNulls || __isset." << (*f_iter)->get_name() << ") {" << endl;
Also, I'm with David on this one. I think the ability to skip nulls is nice, but I wouldn't expect it as the default. I'd probably be okay with committing this before we fix the map/set/list stuff, but only if the default parameter value is false (which I think it should stay as, even after map/set/list is supported).
> Improved toString for Java Structs
> ----------------------------------
>
> Key: THRIFT-119
> URL: https://issues.apache.org/jira/browse/THRIFT-119
> Project: Thrift
> Issue Type: Sub-task
> Components: Compiler (Java)
> Reporter: Bryan Duxbury
> Assignee: Bryan Duxbury
> Attachments: thrift-119.patch
>
>
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (THRIFT-119) Improved toString for Java Structs
Posted by "Bryan Duxbury (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-119?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Bryan Duxbury updated THRIFT-119:
---------------------------------
Component/s: Compiler (Java)
> Improved toString for Java Structs
> ----------------------------------
>
> Key: THRIFT-119
> URL: https://issues.apache.org/jira/browse/THRIFT-119
> Project: Thrift
> Issue Type: Sub-task
> Components: Compiler (Java)
> Reporter: Bryan Duxbury
> Assignee: Bryan Duxbury
> Attachments: thrift-119.patch
>
>
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-119) Improved toString for Java Structs
Posted by "Bryan Duxbury (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-119?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12625860#action_12625860 ]
Bryan Duxbury commented on THRIFT-119:
--------------------------------------
I certainly like Nathan's suggestion from a simplicity standpoint. I think that specifying a field as optional pretty clearly indicates that it should be treated differently.
> Improved toString for Java Structs
> ----------------------------------
>
> Key: THRIFT-119
> URL: https://issues.apache.org/jira/browse/THRIFT-119
> Project: Thrift
> Issue Type: Sub-task
> Components: Compiler (Java)
> Reporter: Bryan Duxbury
> Assignee: Bryan Duxbury
> Attachments: thrift-119.patch
>
>
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (THRIFT-119) Improved toString for Java Structs
Posted by "Bryan Duxbury (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-119?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Bryan Duxbury updated THRIFT-119:
---------------------------------
Attachment: thrift-119-v3.patch
Implemented the suggestion you had above, plus fixed a bug that would make some weird looking strings with leading "," if the first field was null and optional.
> Improved toString for Java Structs
> ----------------------------------
>
> Key: THRIFT-119
> URL: https://issues.apache.org/jira/browse/THRIFT-119
> Project: Thrift
> Issue Type: Sub-task
> Components: Compiler (Java)
> Reporter: Bryan Duxbury
> Assignee: Bryan Duxbury
> Attachments: thrift-119-v2.patch, thrift-119-v3.patch, thrift-119.patch
>
>
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (THRIFT-119) Improved toString for Java Structs
Posted by "Bryan Duxbury (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-119?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Bryan Duxbury updated THRIFT-119:
---------------------------------
Attachment: thrift-119-v2.patch
Here's a version of the patch that always omits optional nulls, without the user being able to pass in an override.
> Improved toString for Java Structs
> ----------------------------------
>
> Key: THRIFT-119
> URL: https://issues.apache.org/jira/browse/THRIFT-119
> Project: Thrift
> Issue Type: Sub-task
> Components: Compiler (Java)
> Reporter: Bryan Duxbury
> Assignee: Bryan Duxbury
> Attachments: thrift-119-v2.patch, thrift-119.patch
>
>
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (THRIFT-119) Improved toString for Java Structs
Posted by "Bryan Duxbury (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-119?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Bryan Duxbury updated THRIFT-119:
---------------------------------
Attachment: thrift-119.patch
Here's the Java version.
The most significant downside of this implementation is that if you call the toString overload with false (to see null optionals), this won't propagate down to all child thrift structs. This could be remedied, but it's a fair amount of additional work, because we end up needing special cases for lists, sets, and maps, since their toString methods don't have an overload that takes a boolean.
> Improved toString for Java Structs
> ----------------------------------
>
> Key: THRIFT-119
> URL: https://issues.apache.org/jira/browse/THRIFT-119
> Project: Thrift
> Issue Type: Sub-task
> Reporter: Bryan Duxbury
> Assignee: Bryan Duxbury
> Attachments: thrift-119.patch
>
>
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-119) Improved toString for Java Structs
Posted by "Bryan Duxbury (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-119?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12625502#action_12625502 ]
Bryan Duxbury commented on THRIFT-119:
--------------------------------------
David, I'm fine with holding off on commit until the propagation works as expected.
Mark, I'll edit that line to make it simpler.
As far as making the default to print out all contents, I don't think that would achieve the desired results. For instance, if you're trying to use an assertEquals in JUnit, it's not going to call the overload, and if they don't match, you're still going to see all the null fields. I guess part of the problem is that I'm treating my struct like a union, but we don't have a separate union type. If we did, this wouldn't be a point of contention, I'd think. The other alternative would be finding a way to support compiler annotations to structs and making this a per-struct option, which would certainly be cool and useful for other reasons.
> Improved toString for Java Structs
> ----------------------------------
>
> Key: THRIFT-119
> URL: https://issues.apache.org/jira/browse/THRIFT-119
> Project: Thrift
> Issue Type: Sub-task
> Components: Compiler (Java)
> Reporter: Bryan Duxbury
> Assignee: Bryan Duxbury
> Attachments: thrift-119.patch
>
>
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-119) Improved toString for Java Structs
Posted by "Mark Slee (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-119?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12633546#action_12633546 ]
Mark Slee commented on THRIFT-119:
----------------------------------
I think this is a reasonable approach. As long as the issets are kept in sync (which they always should be) this will do well.
isset fields are always null. Just write this:
+ indent(out) << "if (__isset." << (*f_iter)->get_name() << " != false) {" << endl;
as this:
+ indent(out) << "if (__isset." << (*f_iter)->get_name() << ") {" << endl;
The "!= false" makes it harder to read, IMO.
> Improved toString for Java Structs
> ----------------------------------
>
> Key: THRIFT-119
> URL: https://issues.apache.org/jira/browse/THRIFT-119
> Project: Thrift
> Issue Type: Sub-task
> Components: Compiler (Java)
> Reporter: Bryan Duxbury
> Assignee: Bryan Duxbury
> Attachments: thrift-119-v2.patch, thrift-119.patch
>
>
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-119) Improved toString for Java Structs
Posted by "Nathan Marz (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-119?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12625528#action_12625528 ]
Nathan Marz commented on THRIFT-119:
------------------------------------
I think the key here is that we only skip *optional* nulls. Obviously, we always want to see the value of every required field. We don't lose any information by not printing out the optional non-set fields - if it's not printed in the list, then the application developer knows that value was not set. I would propose that we don't overload toString at all and make *not* printing optional nulls the only behavior - this will avoid the complexity of maintaining a function that needs to deal with so many special cases (aka, lists, maps, sets, and whatever data structures are added in the future).
> Improved toString for Java Structs
> ----------------------------------
>
> Key: THRIFT-119
> URL: https://issues.apache.org/jira/browse/THRIFT-119
> Project: Thrift
> Issue Type: Sub-task
> Components: Compiler (Java)
> Reporter: Bryan Duxbury
> Assignee: Bryan Duxbury
> Attachments: thrift-119.patch
>
>
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Issue Comment Edited: (THRIFT-119) Improved toString for
Java Structs
Posted by "David Reiss (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-119?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12625456#action_12625456 ]
dreiss edited comment on THRIFT-119 at 8/25/08 11:44 AM:
--------------------------------------------------------------
I think it's pretty confusing to not have it propagate. Would you mind holding off on comitting this until we have a complete implementation?
was (Author: dreiss):
I think it's pretty confusing to not have it propagate. Would you mind holding off on compiling this until we have a complete implementation?
> Improved toString for Java Structs
> ----------------------------------
>
> Key: THRIFT-119
> URL: https://issues.apache.org/jira/browse/THRIFT-119
> Project: Thrift
> Issue Type: Sub-task
> Components: Compiler (Java)
> Reporter: Bryan Duxbury
> Assignee: Bryan Duxbury
> Attachments: thrift-119.patch
>
>
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Resolved: (THRIFT-119) Improved toString for Java Structs
Posted by "David Reiss (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-119?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
David Reiss resolved THRIFT-119.
--------------------------------
Resolution: Fixed
> Improved toString for Java Structs
> ----------------------------------
>
> Key: THRIFT-119
> URL: https://issues.apache.org/jira/browse/THRIFT-119
> Project: Thrift
> Issue Type: Sub-task
> Components: Compiler (Java)
> Reporter: Bryan Duxbury
> Assignee: Bryan Duxbury
> Attachments: thrift-119-v2.patch, thrift-119-v3.patch, thrift-119.patch
>
>
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-119) Improved toString for Java Structs
Posted by "Bryan Duxbury (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-119?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12636117#action_12636117 ]
Bryan Duxbury commented on THRIFT-119:
--------------------------------------
No, definitely unintentional.
> Improved toString for Java Structs
> ----------------------------------
>
> Key: THRIFT-119
> URL: https://issues.apache.org/jira/browse/THRIFT-119
> Project: Thrift
> Issue Type: Sub-task
> Components: Compiler (Java)
> Reporter: Bryan Duxbury
> Assignee: Bryan Duxbury
> Attachments: thrift-119-v2.patch, thrift-119-v3.patch, thrift-119.patch
>
>
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-119) Improved toString for Java Structs
Posted by "David Reiss (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-119?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12625456#action_12625456 ]
David Reiss commented on THRIFT-119:
------------------------------------
I think it's pretty confusing to not have it propagate. Would you mind holding off on compiling this until we have a complete implementation?
> Improved toString for Java Structs
> ----------------------------------
>
> Key: THRIFT-119
> URL: https://issues.apache.org/jira/browse/THRIFT-119
> Project: Thrift
> Issue Type: Sub-task
> Components: Compiler (Java)
> Reporter: Bryan Duxbury
> Assignee: Bryan Duxbury
> Attachments: thrift-119.patch
>
>
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (THRIFT-119) Improved toString for Java Structs
Posted by "Bryan Duxbury (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-119?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Bryan Duxbury updated THRIFT-119:
---------------------------------
Patch Info: [Patch Available]
> Improved toString for Java Structs
> ----------------------------------
>
> Key: THRIFT-119
> URL: https://issues.apache.org/jira/browse/THRIFT-119
> Project: Thrift
> Issue Type: Sub-task
> Reporter: Bryan Duxbury
> Assignee: Bryan Duxbury
> Attachments: thrift-119.patch
>
>
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-119) Improved toString for Java Structs
Posted by "Mark Slee (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-119?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12636113#action_12636113 ]
Mark Slee commented on THRIFT-119:
----------------------------------
Looks good to me. Is the autoconf change related?
> Improved toString for Java Structs
> ----------------------------------
>
> Key: THRIFT-119
> URL: https://issues.apache.org/jira/browse/THRIFT-119
> Project: Thrift
> Issue Type: Sub-task
> Components: Compiler (Java)
> Reporter: Bryan Duxbury
> Assignee: Bryan Duxbury
> Attachments: thrift-119-v2.patch, thrift-119-v3.patch, thrift-119.patch
>
>
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.