Uploaded image for project: 'Thrift'
  1. Thrift
  2. THRIFT-1090

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

Details

    • 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)

      Attachments

        1. thrift-1090.patch
          0.7 kB
          Mike Riley

        Issue Links

          Activity

            yelirekim 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?

            yelirekim 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?
            yelirekim 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.

            yelirekim 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.
            yelirekim Mike Riley added a comment -

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

            yelirekim Mike Riley added a comment - I'm going to reclassify this as a bug so that it can hopefully get some visibility.
            bryanduxbury 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?

            bryanduxbury 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?
            jfarrell 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

            jfarrell 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
            yelirekim 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.

            yelirekim 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.
            yelirekim 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.

            yelirekim 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.
            jfarrell Jake Farrell added a comment -

            Committed, thanks Mike

            jfarrell Jake Farrell added a comment - Committed, thanks Mike
            hudson 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.

            hudson 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.
            jfarrell Jake Farrell added a comment -

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

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

            Reverted? Do you need me to fix that?

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

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

            jfarrell Jake Farrell added a comment - I all ready committed a fix, ran locally and now running against ci to verify
            hudson 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.

            hudson 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.
            jfarrell Jake Farrell added a comment -

            Fixed

            jfarrell Jake Farrell added a comment - Fixed

            People

              yelirekim Mike Riley
              larsfrancke Lars Francke
              Votes:
              3 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved: