Thrift
  1. Thrift
  2. THRIFT-1090

Document the generation of a file called "Constants.java"

    Details

    • Patch Info:
      Patch Available

      Description

      A file called "Constants.java" is generated per thrift file that includes any constants. This should be documented somewhere because if you have multiple thrift files sharing the same namespace there will be clashes.

      (there is no "documentation" component so feel free to adjust those)

        Issue Links

          Activity

          Hide
          Mike Riley added a comment -

          I think really instead of documentation the Java generator shouldn't be clobbering constants defined in separate thrift files.

          To the best of my understanding, most users are defining all constants in one file for each namespace so this doesn't become an issue. Documented or not, it still doesn't seem like a reasonable constraint for how you're allowed to define constants.

          I have this patched in my local copy to just generate a separate .java file containing constants for each program (.thrift file) that contains them. This might not be an ideal solution for everyone but it's how I worked around it.

          Any other suggestions?

          Show
          Mike Riley added a comment - I think really instead of documentation the Java generator shouldn't be clobbering constants defined in separate thrift files. To the best of my understanding, most users are defining all constants in one file for each namespace so this doesn't become an issue. Documented or not, it still doesn't seem like a reasonable constraint for how you're allowed to define constants. I have this patched in my local copy to just generate a separate .java file containing constants for each program (.thrift file) that contains them. This might not be an ideal solution for everyone but it's how I worked around it. Any other suggestions?
          Hide
          Mike Riley added a comment -

          My changes didn't need to be rebased from 0.70 so I included the patch here.

          Again, might not be optimal for everyone but it works.

          Show
          Mike Riley added a comment - My changes didn't need to be rebased from 0.70 so I included the patch here. Again, might not be optimal for everyone but it works.
          Hide
          Mike Riley added a comment -

          I'm going to reclassify this as a bug so that it can hopefully get some visibility.

          Show
          Mike Riley added a comment - I'm going to reclassify this as a bug so that it can hopefully get some visibility.
          Hide
          Bryan Duxbury added a comment -

          This seems like it could be an OK change, though it's important to note that it will break stuff for people using it (since we're changing the class name). Anyone else have an objection to applying this?

          Show
          Bryan Duxbury added a comment - This seems like it could be an OK change, though it's important to note that it will break stuff for people using it (since we're changing the class name). Anyone else have an objection to applying this?
          Hide
          Jake Farrell added a comment -

          Would we be porting this to all other libs? If this is just for java then we should make it optional rather than the default

          Show
          Jake Farrell added a comment - Would we be porting this to all other libs? If this is just for java then we should make it optional rather than the default
          Hide
          Mike Riley added a comment -

          PHP, C++ and Javascript (can't speak for any other languages) already organize constants on a per-program and a per-namespace basis. Cocoa organizes on a per-program basis only because it doesn't have namespaces. Java seems to be the odd man out in organizing per-namespace only.

          Show
          Mike Riley added a comment - PHP, C++ and Javascript (can't speak for any other languages) already organize constants on a per-program and a per-namespace basis. Cocoa organizes on a per-program basis only because it doesn't have namespaces. Java seems to be the odd man out in organizing per-namespace only.
          Hide
          Mike Riley added a comment -

          Should I re-implement this as a language option? From my perspective this is an inconsistency that Java has when compared to the other languages: constants can't be defined in different programs having the same namespace. On the other hand, if there is a consensus that it should be added as an option it wouldn't be so difficult to implement.

          Show
          Mike Riley added a comment - Should I re-implement this as a language option? From my perspective this is an inconsistency that Java has when compared to the other languages: constants can't be defined in different programs having the same namespace. On the other hand, if there is a consensus that it should be added as an option it wouldn't be so difficult to implement.
          Hide
          Jake Farrell added a comment -

          Committed, thanks Mike

          Show
          Jake Farrell added a comment - Committed, thanks Mike
          Hide
          Hudson added a comment -

          Integrated in Thrift #347 (See https://builds.apache.org/job/Thrift/347/)
          Thrift-1090: Document the generation of a file called "Constants.java"
          Client: java
          Patch: Mike Riley

          Adds program_name to the Constants file in java to match all other client libs.

          Show
          Hudson added a comment - Integrated in Thrift #347 (See https://builds.apache.org/job/Thrift/347/ ) Thrift-1090: Document the generation of a file called "Constants.java" Client: java Patch: Mike Riley Adds program_name to the Constants file in java to match all other client libs.
          Hide
          Jake Farrell added a comment -

          Patch did not take into account test cases that used Constants file

          Show
          Jake Farrell added a comment - Patch did not take into account test cases that used Constants file
          Hide
          Mike Riley added a comment -

          Reverted? Do you need me to fix that?

          Show
          Mike Riley added a comment - Reverted? Do you need me to fix that?
          Hide
          Jake Farrell added a comment -

          I all ready committed a fix, ran locally and now running against ci to verify

          Show
          Jake Farrell added a comment - I all ready committed a fix, ran locally and now running against ci to verify
          Hide
          Hudson added a comment -

          Integrated in Thrift #350 (See https://builds.apache.org/job/Thrift/350/)
          Thrift-1090: Document the generation of a file called "Constants.java"
          Client: java
          Patch: Jake Farrell

          Fixing test cases left out in initial patch.

          Show
          Hudson added a comment - Integrated in Thrift #350 (See https://builds.apache.org/job/Thrift/350/ ) Thrift-1090: Document the generation of a file called "Constants.java" Client: java Patch: Jake Farrell Fixing test cases left out in initial patch.
          Hide
          Jake Farrell added a comment -

          Fixed

          Show
          Jake Farrell added a comment - Fixed

            People

            • Assignee:
              Mike Riley
              Reporter:
              Lars Francke
            • Votes:
              3 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development