Support Scala Native #826
Support Scala Native #826
Conversation
lolgab
force-pushed
the
support-scala-native
branch
2 times, most recently
from
c9e49fd
to
00b6ff6
Compare
lolgab
force-pushed
the
support-scala-native
branch
from
79b392e
to
d5d70f0
Compare
|
|
||
| import java.io.InputStreamReader | ||
|
|
||
| private[json] object StaticBindingJsNative { |
There was a problem hiding this comment.
The code in this file was moved from StaticBinding in the js implementation.
There was a problem hiding this comment.
I'm just curious here - what's the point of this restructure?
There was a problem hiding this comment.
Ah, I think I see - you're making the underlying functionality common, so that both js and native StaticBinding implementations can access this code?
There was a problem hiding this comment.
Ah, I think I see - you're making the underlying functionality common, so that both
jsandnativeStaticBindingimplementations can access this code?
Exactly, to avoid copy-pasting a lot of code, I put the common code here so it's shared and called back and forth from the Native and JS implementations.
There was a problem hiding this comment.
A small nit then - I found the js-native and StaticBindingJsNative names a bit confusing. The name doesn't really convey the intent, and isn't intuitive for further possible expansions.
There was a problem hiding this comment.
the js-native directory is automatically added by sbt-crossproject.
About StaticBindingJsNative I just added the platforms names to the original file name. If you have a better suggestion I change it. I don't have any better name in mind.. maybe StaticBindingNonJvm ?
There was a problem hiding this comment.
Ah, I had a feeling it was sbt plugin shenanigans causing the package name. Makes sense, given that the subprojects aren't explicitly depending on one another in build.sbt.
I think it's really a nit though, I hate to delay for this. Let's leave it as is, and leave better naming for later. This isn't a public API, so it shouldn't affect very many users.
I will say though, StaticBindingNonJvm or StaticBindingNonJvmShared is a bit more descriptive.
| @@ -0,0 +1,58 @@ | |||
| /* | |||
| * Copyright (C) 2009-2021 Lightbend Inc. <https://www.lightbend.com> | |||
There was a problem hiding this comment.
I figure this is needed to ensure linting works on the new rules. But this isn't really associated with Lightbend anymore. @mkurz we should visit revamping the requirements on copyrights and file headers.
There was a problem hiding this comment.
Approved, though we should track the comments I made.
- Naming of
StaticBindingJsNativefeels very specific, and doesn't match the intent necessarily. - We're adding files and putting outdated Lightbend copyright data at the top, just to satisfy CI. We should do something different in the future.
|
@mkurz I'll hold off on merging, wanted to give you another day to come back and make sure I didn't miss anything. I'll merge tomorrow if you have no other thoughts. |
|
will take a look tonight |
|
@BillyAutrey I can rename |
|
@lolgab up to you! Like I said I feel it's a nit, and an easy one to fix later if we want to discuss it further. |
Since we are waiting for tomorrow either way, it doesn't cost me anything to update it :) |
|
Thank you very much for reviewing and approving the PR! |
|
Looking into this the next minutes, thanks for your patience. |
mkurz
force-pushed
the
support-scala-native
branch
from
12b53ff
to
f05f1ce
Compare
There was a problem hiding this comment.
LGTM, rebased and upgraded versions, since we are now on Scala 3.3.0-RC5.
|
I published |
Pull Request Checklist
Purpose
Add support for Scala Native