-
Notifications
You must be signed in to change notification settings - Fork 268
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Drop compat code for java 8 #2155
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's get this in now, we could have gotten rid of this a while ago already
There is actually a catch, as c848cfe shows. |
I'm interested in your opinion on |
de8bc92
to
e43bcdd
Compare
Since this isn't trivial after all, I think it should wait for after the release. |
Foo.Bar -> "Bar", | ||
Foo.Baz(42) -> "Baz" | ||
).foreach { case (o, ref) => assert(o.getClass.getPrettySimpleName == ref) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: indent is weird
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 90a9fdc.
Should we rebase and merge this? |
It class `.getSimpleName` and strips the '$' character of object classes. Also, we don't need the java 8 workaround for `.getSimpleName`
e43bcdd
to
90a9fdc
Compare
I rebased in 90a9fdc, but the risk of regression exists and I'm hesitant to merge immediately. |
It seems like java 11 is getting near its EOL (so we should update our documentation to rely on java 17), does that impact our choice of whether or not to merge this? I haven't thoroughly tested it, but I can take a look at it if we want another set of eyes to figure out what the risk of regression is. |
It has been a long time and officially supported version is Java 11.
This can wait after the upcoming release though.