Generify AttributeRenderer #233
Merged
Conversation
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
Add a generic type on the AttributeRenderer interface and make methods using it generic, too. This allows subclasses like NumberRenderer or StringRenderer to not have to use casts. The change remains full binary compatibility because of erasure, and source compatibility for valid uses. Code like ``` group.registerRenderer(Object.class, new StringRenderer()); ``` now produces a compiler error. It would previously have failed at runtime due to the forced (String)o cast.
7a58ee7 to
1162f7f
Compare
Member
|
Wow! Interesting change. Let me consider this. |
Member
|
@sharwell do you think it's safe to add generics here? Is it worth it? I.e., is there really a problem we're solving here? |
Member
|
I think the concept is cool but I'd like to review it when I'm back at a computer to see if we can improve accuracy of some of the implementation details. |
sharwell
suggested changes
Dec 28, 2019
Member
sharwell
left a comment
There was a problem hiding this comment.
I completed my initial review. I left a few comments to help improve accuracy of the type signatures involved in the change. After completing these changes (and confirming that the code follows the best patterns possible given our compatibility constraints), I would like to see ModelAdaptor updated in the same way so the new API is consistent.
Co-Authored-By: Sam Harwell <sam@tunnelvisionlabs.com>
Co-Authored-By: Sam Harwell <sam@tunnelvisionlabs.com>
Co-Authored-By: Sam Harwell <sam@tunnelvisionlabs.com>
…cept Class<? extends T> again
sharwell
approved these changes
Dec 29, 2019
Member
|
Thanks alot, @sharwell ! |
Member
|
@Clashsoft would you be willing to submit a pull request to make the same change for |
Clashsoft
added a commit
to Clashsoft/stringtemplate4
that referenced
this pull request
Dec 30, 2019
As suggested in antlr#233 The only change introduced by this that can be considered breaking is the removal of the superclass MapModelAdaptor from AggregateModelAdaptor. The inheritance was replaced by composition. This had to be done because MapModelAdaptor is now a ModelAdaptor<Map>, while AggregateModelAdaptor needs to be a ModelAdaptor<Aggregate>. Previously, AggregateModelAdaptor could not handle a Map, so I think this "breaking" change should not cause any issues.
This was referenced Dec 30, 2019
Merged
Clashsoft
added a commit
to Clashsoft/stringtemplate4
that referenced
this pull request
Jan 8, 2020
Merged
Clashsoft
added a commit
to Clashsoft/stringtemplate4
that referenced
this pull request
Jan 8, 2020
Clashsoft
added a commit
to Clashsoft/stringtemplate4
that referenced
this pull request
Jan 11, 2020
Reverts NumberRenderer and StringRenderer to accept Object instead of Number or String respectively. NumberRenderer always worked with Object before. Actually it should be called FormatRenderer or similar because it merely uses Formatter, nothing Number-specific. StringRenderer now uses the pre-antlr#233 behaviour of accepting Object at runtime, but failing if the actual value is not a String due to the forced cast.
parrt
added a commit
that referenced
this pull request
Jan 11, 2020
Fix backward compatibility issues introduced by #233
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Add a generic type variable on the AttributeRenderer interface and make methods using it generic, too.
This allows subclasses like NumberRenderer or StringRenderer to not have to use casts.
The change retains full binary compatibility because of erasure, and source compatibility for valid uses. Code like
now produces a compiler error. It would previously have failed at runtime due to the forced (String)o cast.