-
Notifications
You must be signed in to change notification settings - Fork 14
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
Set up Scalafmt and Scalafix and other small improvements #39
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.
Sorry for the terse review. Don't have a lot of time but trying to unblock you.
/** Reflectively looks up a loadable module class. | ||
|
||
/** | ||
* Reflectively looks up a loadable module class. |
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 is quite obviously wrong.
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.
Not obvious to me, since I have almost zero knowledge about Scala.js (this is the JS part of Reflect, right?) 😁
Or did you mean the comment formatting?
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.
yes, I meant the formatting :) (see too terse).
the lower comments are indented by more. further, I think this repo follows Scala.js style:
https://github.com/scala-js/scala-js/blob/master/CODINGSTYLE.md
There have been quite some attempts to auto-format it, often the formatters were too buggy. Last attempt:
scala-js/scala-js#4260
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.
Now I see what's going on.
The Scala.js guideline says it should be
/** Reflectively ...
But Scalafmt keeps changing it to
/**
* Reflectively
even though we have preset=Scala.js
. That's a bit annoying, I have to say 😕 But IMHO still much better than no automatic Scalafmt formatting at all 👍
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.
The "Scala.js" preset was defined 5 years ago when scalafmt was in its infancy. It is completely obsolete and several more options are required to get close to the Scala.js style. You can have a look at the most recent PRs trying to enable scalafmt in the core Scala.js codebase for inspiration.
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.
I see! Reformatted ✔️
jvm/src/main/scala/org/portablescala/reflect/InstantiatableClass.scala
Outdated
Show resolved
Hide resolved
Terse reviews are the best -- quick to read and act upon 😸 Thank you Tobias! |
83688b3
to
b36a036
Compare
Or would you @sjrd prefer to adopt the Scalafmt configuration from Scala Native (scala-native/scala-native#2315), i.e. # Overall we are trying to use default settings to the
# maximum extent possible. Removing non-default
# settings is preferred over adding especially as
# the Scala language evolves and styles change.
# Test upgrades: $ scripts/scalafmt --test 2> diff.txt
version = "3.0.0-RC5"
docstrings.style = AsteriskSpace
project.git = true
project.excludePaths = [
"glob:**/scalalib/**"
]
# This creates less of a diff but is not default
# but is more aligned with Scala.js syntax.
newlines.beforeCurlyLambdaParams = multilineWithCaseOnly
# Keep control sites more streamlined
indent.ctrlSite = 4
danglingParentheses.ctrlSite = false
# default value is 10,000
# Message about NirGenExpr.scala goes away between 18k and 20k
runner.optimizer.maxVisitsPerToken = 20000
rewriteTokens = {
"⇒": "=>"
"→": "->"
"←": "<-"
} |
Update versions and other small improvements.