Thrift
  1. Thrift
  2. THRIFT-477

remove extra methods generated for collections

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Trivial Trivial
    • Resolution: Invalid
    • Affects Version/s: 0.1
    • Fix Version/s: 0.2
    • Component/s: Java - Compiler
    • Labels:
      None
    • Patch Info:
      Patch Available

      Description

      For some reason, the java generator produces a bunch of extra methods whenever you have a field of a collection type. For instance if you have a field called "my_list", then you will get a "get_my_list_size()" method in addition to the standard getter and setter.

      Personally I find these extra methods redundant, since all they save you is a pair of parentheses. I'd prefer to just get rid of them. Does anyone out there like or use these methods?

      1. thrift-477.patch
        3 kB
        Bryan Duxbury

        Activity

        Hide
        Todd Lipcon added a comment -

        resolving invalid since these changes were rolled back prior to 0.2 release

        Show
        Todd Lipcon added a comment - resolving invalid since these changes were rolled back prior to 0.2 release
        Hide
        Bryan Duxbury added a comment -

        I rolled back the changes.

        Show
        Bryan Duxbury added a comment - I rolled back the changes.
        Hide
        Mathias Herberts added a comment -

        This is issue is marked fixed despite the latest comments by Jarski requesting the rollback of those methods, what is its status?

        I'm convinced too that those methods render the code using Thrift cleaner and smaller as they take care of field initialization.

        Show
        Mathias Herberts added a comment - This is issue is marked fixed despite the latest comments by Jarski requesting the rollback of those methods, what is its status? I'm convinced too that those methods render the code using Thrift cleaner and smaller as they take care of field initialization.
        Hide
        Jarski added a comment -

        I support what David suggests - turn on the new behavior with a compiler switch. The reasoning behind this: the putToX() and addToX() methods are useful enough in their own right (and not just redundant code), that they could be useful for new projects using thrift. If it was a non-default these would probably go undiscovered.

        Show
        Jarski added a comment - I support what David suggests - turn on the new behavior with a compiler switch. The reasoning behind this: the putToX() and addToX() methods are useful enough in their own right (and not just redundant code), that they could be useful for new projects using thrift. If it was a non-default these would probably go undiscovered.
        Hide
        David Reiss added a comment -

        What about making the new behavior accessible by a compiler switch?

        Show
        David Reiss added a comment - What about making the new behavior accessible by a compiler switch?
        Hide
        Bryan Duxbury added a comment -

        What about making the old behavior accessible by a compiler switch?

        Show
        Bryan Duxbury added a comment - What about making the old behavior accessible by a compiler switch?
        Hide
        David Reiss added a comment -

        If people find them useful, I think we should add them back. The cost of having them is minimal.

        Show
        David Reiss added a comment - If people find them useful, I think we should add them back. The cost of having them is minimal.
        Hide
        Jarski added a comment -

        Sorry for not noticing this discussion earlier, but our company just upgraded thrift and it turns out we were relying heavily on this. Do you guys object if we keep these, so that we don't have to un-patch this for our internal compiler builds? The redundant BlahSize() methods we don't care much for either, but the putToX() and addToX() methods were particularly useful since they do a null check + automatically instantiate the corresponding collection if it doesn't exist.

        Show
        Jarski added a comment - Sorry for not noticing this discussion earlier, but our company just upgraded thrift and it turns out we were relying heavily on this. Do you guys object if we keep these, so that we don't have to un-patch this for our internal compiler builds? The redundant BlahSize() methods we don't care much for either, but the putToX() and addToX() methods were particularly useful since they do a null check + automatically instantiate the corresponding collection if it doesn't exist.
        Hide
        Bryan Duxbury added a comment -

        Since there was never any response from Dave Engberg, I am assuming he is no longer following Thrift. As such, I committed this.

        Show
        Bryan Duxbury added a comment - Since there was never any response from Dave Engberg, I am assuming he is no longer following Thrift. As such, I committed this.
        Hide
        Bryan Duxbury added a comment -

        Does the elusive Dave Engberg read the mailing list?

        Show
        Bryan Duxbury added a comment - Does the elusive Dave Engberg read the mailing list?
        Hide
        David Reiss added a comment -

        I'm okay with it if Dave Engberg is.

        Show
        David Reiss added a comment - I'm okay with it if Dave Engberg is.
        Hide
        Bryan Duxbury added a comment -

        This patch removes the special extra methods for collections. What do people think about committing this?

        Show
        Bryan Duxbury added a comment - This patch removes the special extra methods for collections. What do people think about committing this?
        Hide
        David Reiss added a comment -

        These methods were in the original Javabean patches, contributed by Dave Engberg. If he doesn't want them, I'm fine with dropping them.

        Show
        David Reiss added a comment - These methods were in the original Javabean patches, contributed by Dave Engberg. If he doesn't want them, I'm fine with dropping them.

          People

          • Assignee:
            Unassigned
            Reporter:
            Bryan Duxbury
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development