This repository was archived by the owner on May 3, 2022. It is now read-only.
Conversation
## Problem The classes generated for this file in Java by thrift compiler are currently put in default package, making them unusable from any other Java package. This also means that any other thrift files that `include` this one, but declare a `java namespace` will have their thrift compilation successful, error will only be surfaced by Java compiler when those thrift files are used in Java project making chain of updates very costly. One example of a workaround is in [separate copy of this file](https://github.com/uber/tchannel-java/blob/771a42174896f6f6ca7fbb6e37a2c227cda3f3b5/tchannel-core/src/main/thrift/meta.thrift) in Java tchannel library. ## Backwards compatibility concerns I don't expect this diff breaking anything because the generated files could only be referenced from default package and that is not a widely spread practice in Java to begin with. However, repos that commit generated files in the artifacts will have a lot of their code regenerated.
|
|
kriskowal
reviewed
Aug 21, 2020
Contributor
kriskowal
left a comment
kriskowal
left a comment
There was a problem hiding this comment.
Resigning from review.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The classes generated for this file in Java by thrift compiler are currently put in default package, making them unusable from any other Java package.
This also means that any other thrift files that
includethis one, but declare ajava namespacewill have their thrift compilation successful, error will only be surfaced by Java compiler when those thrift files are used in Java project making chain of updates very costly.One example of a workaround is in separate copy of this file in Java tchannel library.
Backwards compatibility concerns
I don't expect this diff breaking anything because the generated files could only be referenced from default package and that is not a widely spread practice in Java to begin with.
However, repos that commit generated files in the artifacts will have a lot of their code regenerated.