-
Notifications
You must be signed in to change notification settings - Fork 278
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
Update readme.md to add JDK compatibility #4714
base: main
Are you sure you want to change the base?
Conversation
readme.md
Outdated
@@ -24,6 +24,12 @@ Head over to [the user docs][docs] for instructions on how to install scalafmt. | |||
`browser-sync start --server --files "target/*.html"`. | |||
See [Browsersync](https://www.browsersync.io/). | |||
|
|||
### JDK compatibility |
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.
there's an installation section... in fact, entire file on installation in docs/
.
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.
Thanks for pointing this out!
Would you prefer if I move this change to docs/installation.md
?
Edit: the response is yes (from your other message).
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!
As mentioned, docs/installation.md might be a better file for this.
is this just an unrelated, side comment? I don't see any change associated with it. |
Yes this is a side comment as I am not sure how to resolve it: delete the badge altogether or something else. |
Just pushed changes:
|
@@ -5,6 +5,12 @@ title: Installation | |||
|
|||
You can use Scalafmt from your editor, build tool or terminal. | |||
|
|||
## JDK compatibility |
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.
perhaps this is a strange question... which method of using scalafmt
does this pertain to?
is it sbt
, a standalone binary with coursier
(instructions below), etc? or all of them?
does it apply to the actual integration one uses, or the dynamic version
within .scalafmt.conf
, or both?
in either case, it would be good to clarify it here.
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 PR was motivated by sbt-scalafmt
as I mainly use it (and seldomly the binary version).
I failed to exhibit issues running the latest dynamic
version on Java 8. The only scenario that causes me problems is when publishing a plugin based on latest sbt-scalafmt
. The plugin contains a transitive dependency to com.github.plokhotnyuk.jsoniter-scala
that is compiled for Java 11 (class version 55). I am not familiar with jsoniter-scala
but it looks to me that this is a compile-time only dependency. This seems to be confirmed by excluding the library from the import; Sbt builds correctly and my projects passes the unit-tests.
I'm aware this is not a direct answer to your remarks but could it be that scalafmt (as a whole) still supports Java 8 and that the only obstacle is a dependency that may not be required at runtime?
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.
strange... if scalameta does not support java 8, how can scalafmt (which uses scalameta) be ok with java 8?
also, have you tried running the latest static scalafmt using java 8?
Similar to scalameta/sbt-scalafmt#341 (here the section is h3 title because there is not
installation
section).The second badge (CI appveyor) is broken. It seems the project does not use this building service anymore.