-
Notifications
You must be signed in to change notification settings - Fork 181
Replace Lightbend Config with sconfig #2187
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
base: main
Are you sure you want to change the base?
Conversation
6399882
to
7685ff1
Compare
6234fa5
to
6995b93
Compare
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.
This was the only non mechanical change needed, it was basically updating the apiMappings
to point to the new online documentation for sconfig. Since sconfig is written in Scala we need to pass in the scala binary version to the doc URI
@mkurz I know you were hesitant with this change but here is the PR with the changeset if you are curious on the difficulty/problems of doing such a change (with pekko so far there wasn't any problems at all) |
Technically, this all seems fine. The main issue is that we need consensus on whether this change is needed. Even if we can persuade the sconfig team to have succession planning or possibly, setting up an ASF podling, this will take time. |
All valid points, lets continue discussing them at #2186. The primary purpose of making this PR now was to figure out how technically difficult it was (in reality not at all) and if there were any problems (none so far) |
Waiting until all features is ready and battle tested, and still a vote pass. And still a new version dropped the ccompat |
ccompat is a non issue as we already have ccompat in our code, its just manually inlined. This was done many many years ago before ccompat was even stable but thats no longer the case. |
6995b93
to
73386ea
Compare
The cc module will be removed once 2.12 is removed. |
I will follow @mkurz 's decision on this pr, if Play think we should stay , we should. |
73386ea
to
cc465e5
Compare
So I have some good news, with the merge of ekrich/sconfig#488 on the next release of Turns out that having |
cc465e5
to
f9d18e5
Compare
Does the sconfig fix all the pending issues in the lightbend/config ? |
This PR replaces Lightbend Config with sconfig, for rationale please see discussion at #2186.
99% of the changes were entirely mechanical due to the fact that aside from the package, sconfig is entirely source compatible with lightbend config while also keeping the same behaviour. More specifically
com.typesafe.config
toorg.ekrich.config
the source code is 100% the same. That means for our Java users, if they ever programmatically used the typesafe config all they need to do is to change an importcom.typesafe.config
toorg.ekrich.config
), newer versions of Scala have deprecated auto unary tupling and auto unary untupling, i.eConfigFactory.empty
vsConfigFactory.empty()
andv.valueType()
vsv.valueType
. Although we can technically make our Scala codebase compile without any changes of adding/removing()
, this would involve suppressing deprecation warnings so I don't see any benefit in doing so.apiMappings
, this has been noted in the PR so its clear to see.I still need to do a manual pass to check the documentation generation works fine (paradox compiled without issues but I will manually generate it locally to double check that the links to sconfig docs work as expected). Due to the large number of changes if you want to manually review the PR I would recommend checking out the branch and doing it locally.
To keep the discussions on topic, if you have questions on the premise of the PR (i.e. using sconfig in the first place) then please use the discussion thread at #2186. On the other hand if you have specific discussion regarding the implementation in this PR then feel free to comment