Skip to content
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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Drop compat code for java 8 #2155

wants to merge 1 commit into from

Conversation

pm47
Copy link
Member

@pm47 pm47 commented Jan 27, 2022

It has been a long time and officially supported version is Java 11.

This can wait after the upcoming release though.

@pm47 pm47 requested a review from t-bast January 27, 2022 15:17
t-bast
t-bast previously approved these changes Jan 27, 2022
Copy link
Member

@t-bast t-bast left a 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

@pm47
Copy link
Member Author

pm47 commented Jan 27, 2022

There is actually a catch, as c848cfe shows. getSimpleClassName actually returns the full or simple class name depending on whether it is applied to an object or a class!

@pm47
Copy link
Member Author

pm47 commented Jan 27, 2022

I'm interested in your opinion on obj.getClass.getPrettySimpleName vs obj.prettyClassName.

@t-bast
Copy link
Member

t-bast commented Jan 27, 2022

Since this isn't trivial after all, I think it should wait for after the release.

t-bast
t-bast previously approved these changes Feb 2, 2022
Foo.Bar -> "Bar",
Foo.Baz(42) -> "Baz"
).foreach { case (o, ref) => assert(o.getClass.getPrettySimpleName == ref)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: indent is weird

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 90a9fdc.

@t-bast
Copy link
Member

t-bast commented Sep 18, 2024

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`
@codecov-commenter
Copy link

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 72.54902% with 14 lines in your changes missing coverage. Please review.

Project coverage is 86.27%. Comparing base (14a4ea4) to head (90a9fdc).
Report is 18 commits behind head on master.

Files with missing lines Patch % Lines
...in/scala/fr/acinq/eclair/channel/fsm/Channel.scala 72.72% 3 Missing ⚠️
...inq/eclair/channel/fsm/ChannelOpenDualFunded.scala 0.00% 3 Missing ⚠️
...q/eclair/payment/receive/MultiPartPaymentFSM.scala 0.00% 2 Missing ⚠️
...ain/scala/fr/acinq/eclair/router/FrontRouter.scala 77.77% 2 Missing ⚠️
...cala/fr/acinq/eclair/crypto/TransportHandler.scala 50.00% 1 Missing ⚠️
...c/main/scala/fr/acinq/eclair/io/MessageRelay.scala 0.00% 1 Missing ⚠️
...cala/fr/acinq/eclair/remote/ScodecSerializer.scala 0.00% 1 Missing ⚠️
...ir-front/src/main/scala/fr/acinq/eclair/Boot.scala 0.00% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2155      +/-   ##
==========================================
+ Coverage   85.98%   86.27%   +0.29%     
==========================================
  Files         219      224       +5     
  Lines       18538    19846    +1308     
  Branches      786      807      +21     
==========================================
+ Hits        15939    17123    +1184     
- Misses       2599     2723     +124     
Files with missing lines Coverage Δ
...re/src/main/scala/fr/acinq/eclair/NodeParams.scala 91.73% <100.00%> (-1.75%) ⬇️
...in/scala/fr/acinq/eclair/channel/Commitments.scala 96.83% <ø> (ø)
...la/fr/acinq/eclair/channel/fsm/ErrorHandlers.scala 82.69% <100.00%> (-0.87%) ⬇️
...c/main/scala/fr/acinq/eclair/db/pg/PgAuditDb.scala 98.23% <ø> (-1.47%) ⬇️
...ain/scala/fr/acinq/eclair/db/pg/PgPaymentsDb.scala 98.61% <100.00%> (ø)
...cala/fr/acinq/eclair/db/sqlite/SqliteAuditDb.scala 98.11% <ø> (-1.56%) ⬇️
...a/fr/acinq/eclair/db/sqlite/SqlitePaymentsDb.scala 98.63% <100.00%> (ø)
...src/main/scala/fr/acinq/eclair/io/Monitoring.scala 100.00% <ø> (ø)
...n/scala/fr/acinq/eclair/json/JsonSerializers.scala 96.42% <ø> (ø)
...-core/src/main/scala/fr/acinq/eclair/package.scala 73.52% <100.00%> (ø)
... and 18 more

... and 6 files with indirect coverage changes

@pm47
Copy link
Member Author

pm47 commented Sep 26, 2024

Should we rebase and merge this?

I rebased in 90a9fdc, but the risk of regression exists and I'm hesitant to merge immediately.

@t-bast
Copy link
Member

t-bast commented Sep 30, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants