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

Set up Scalafmt and Scalafix and other small improvements #39

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

Conversation

sideeffffect
Copy link
Contributor

@sideeffffect sideeffffect commented Mar 5, 2021

Update versions and other small improvements.

@sideeffffect sideeffffect changed the title Facelift [WIP] Facelift Mar 5, 2021
@sideeffffect sideeffffect reopened this Mar 5, 2021
@sideeffffect sideeffffect changed the title [WIP] Facelift Facelift Mar 5, 2021
@sideeffffect
Copy link
Contributor Author

Hello, could somebody please have a look at this PR? 🙏
/cc @sjrd @gzm0

Copy link
Contributor

@gzm0 gzm0 left a 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.
Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Contributor

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

Copy link
Contributor Author

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 👍

Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see! Reformatted ✔️

js/src/main/scala/org/portablescala/reflect/Reflect.scala Outdated Show resolved Hide resolved
@sideeffffect
Copy link
Contributor Author

Terse reviews are the best -- quick to read and act upon 😸 Thank you Tobias!

@sideeffffect sideeffffect requested a review from gzm0 March 31, 2021 10:48
@sideeffffect sideeffffect requested a review from gzm0 April 1, 2021 19:28
@sideeffffect
Copy link
Contributor Author

I've merged in the newest master branch containing the fix for InstantiatableClass using getDeclaredConstructor.
Could you please give this another round of review? @gzm0 @sjrd

@sideeffffect sideeffffect requested a review from sjrd April 4, 2021 00:09
@sideeffffect sideeffffect force-pushed the facelift branch 2 times, most recently from 83688b3 to b36a036 Compare April 4, 2021 01:41
@sideeffffect sideeffffect changed the title Facelift Set up Scalafmt and Scalafix and other small improvements Jul 1, 2021
@sideeffffect
Copy link
Contributor Author

@gzm0 @sjrd could you please give this another look? 🙏

@sideeffffect
Copy link
Contributor Author

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 = {
  "⇒": "=>"
  "→": "->"
  "←": "<-"
}

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