[BEAM-59] Switch mimeType from mutable protected field to constructor #2793
Conversation
This file contains 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
Protected mutable fields are a terrible design pattern
|
R: @lukecwik maybe you'll have thoughts on this one? |
lukecwik
reviewed
May 1, 2017
| @@ -1026,11 +1024,15 @@ public void verifyDeterministic() throws NonDeterministicException { | |||
| WritableByteChannel create(WritableByteChannel channel) throws IOException; | |||
|
|
|||
| /** | |||
| * @return the MIME type that should be used for the files that will hold the output data | |||
| * Returns the MIME type that should be used for the files that will hold the output data. May | |||
| * return {@code null} if this {@code WritableByteChannelFactory} does not meaningful change | |||
| */ | ||
| protected String mimeType = MimeTypes.TEXT; | ||
| private final String mimeType; |
There was a problem hiding this comment.
Keep a comment that this is the mimetype of the sink and that it can be overridden by the WritableByteChannelFactory
| */ | ||
| protected String mimeType = MimeTypes.TEXT; | ||
| private final String mimeType; | ||
|
|
||
| /** | ||
| * Construct a new FileBasedWriter with a base filename. |
There was a problem hiding this comment.
Add a comment about the resolution order of mime types.
dhalperi
commented
May 1, 2017
| */ | ||
| protected String mimeType = MimeTypes.TEXT; | ||
| private final String mimeType; |
| */ | ||
| protected String mimeType = MimeTypes.TEXT; | ||
| private final String mimeType; | ||
|
|
||
| /** | ||
| * Construct a new FileBasedWriter with a base filename. |
| @@ -1026,11 +1024,15 @@ public void verifyDeterministic() throws NonDeterministicException { | |||
| WritableByteChannel create(WritableByteChannel channel) throws IOException; | |||
|
|
|||
| /** | |||
| * @return the MIME type that should be used for the files that will hold the output data | |||
| * Returns the MIME type that should be used for the files that will hold the output data. May | |||
| * return {@code null} if this {@code WritableByteChannelFactory} does not meaningful change | |||
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Protected mutable fields are a terrible design pattern, and this
information is known at construction time.
Also fix a minor bug in which the Binary MIME type of a FileBasedSink could erroneously be changed back to a Text MIME type when using an uncompressed channel.