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

Regression test for formatting changes #154

Closed
olafurpg opened this issue Apr 13, 2016 · 13 comments
Closed

Regression test for formatting changes #154

olafurpg opened this issue Apr 13, 2016 · 13 comments

Comments

@olafurpg
Copy link
Member

Great suggestion from @fommil on gitter:

A regression test on these same projects [ 10k source files from Scala.js, Akka, Spark and a few more projects] using the sbt plugin would be great. Run the formatter, generate a diff, assert there are no changes
Commit the changes that you feel are acceptable (i.e. bugs in scalariform)
So they are white listed

I'm already manually doing this for scalafmt.

@fommil
Copy link

fommil commented Apr 13, 2016

I recommend using sbt's scripted for this. I have some examples, e.g. ensime-sbt or sbt-big-project. The former actually has a git subtree of ensime-server in it and no reason you couldn't do that for akka/spark/etc (it makes your repo a lot larger though)

@olafurpg
Copy link
Member Author

I hadn't heard of sbt scripted, sounds like a perfect fit for our needs.

no reason you couldn't do that for akka/spark/etc (it makes your repo a lot larger though)

I think it's unnecessary to add so many sub-repos. Instead, we can

  • have a single separate github repo, which uses the scalafmt sbt plugin and contains a snapshot of all scala files in akka/spark/intellij/..., making sure to include appropriate licences
  • the repo's master branch would always be formatted to the latest approved/stable release of scalafmt, adding appropriate tags at each new release.
  • the regression test clones that repo and runs the scalafmtTest sbt task from within the project, which exits with an exception in case of mis-formatted code.

@fommil
Copy link

fommil commented Apr 13, 2016

please add ensime-server to the list of projects to test, we use scalariform

@olafurpg
Copy link
Member Author

olafurpg commented May 13, 2016

Here's a WIP repository to see diffs between scalafmt releases: https://github.com/olafurpg/scala-repos/compare/0.2.3...0.2.4?expand=1

I haven't added ensime-server yet. The repo is probably too big at the moment, the diff alone is almost ~5mb. However, it only took ~2 minutes to format 3.2 million LOC 😄

@fommil
Copy link

fommil commented May 13, 2016

cool! Really keen on this. If it works for something as big as akka, then we're golden.

@olafurpg
Copy link
Member Author

I will never release again. Seeing so many bugs in the diff I have to fix first!

More seriously, though, this is great. I'm sold on publishing a diff for each release to show users what they can expect.

@olafurpg
Copy link
Member Author

I think the corpus I'm currently using is too big. It contains ~3 million lines of code and the diffs are so huge (see olafurpg/scala-repos#2) that I can't be bothered to go through all of it. I think it would be smart to hand pick a ~1.000 file subset of those files.

@olafurpg olafurpg added this to the 0.3 milestone Jun 23, 2016
@olafurpg olafurpg removed this from the 0.4 milestone Sep 27, 2016
@olafurpg
Copy link
Member Author

0.4 introduced no regression diff from 0.3.1 🎉 Infrastructure to run these tests automatically before every release would sure be nice ^^

@fommil
Copy link

fommil commented Sep 27, 2016

Actually I meant to talk to you about scalafmt at scala world but so much else to do. I'm thinking of removing b scalariform from ensime-server and making all the functionality we need available as an sbt plugin. It would be fantastic the me able to let the user swap it out for scalafmt. The only feature we need right now is format single file, is that possible? If we could get format region (e.g. by providing start/end positions) that would be even better. Mutating the files of disk is preferred.

@olafurpg
Copy link
Member Author

The only feature we need right now is format single file, is that possible?

Of course! That's the core feature ;) You probably want to use scalafmt as a standalone library. The "quick start" docs are here: https://olafurpg.github.io/scalafmt/#Standalonelibrary
I recommend you use

org.scalafmt.config.Config.fromHocon(<contents of .scalafmt.conf file>)

to get a ScalafmtConfig instance. Docs on how configure .scalafmt.conf are here: https://olafurpg.github.io/scalafmt/#Configuration

The .scalafmt.conf file should be in the root dir of the project, I advise against having a custom config setup for ENSIME since this would complicate integrations with SBT and IntelliJ.

Note that scalafmt is 2.11 only, I don't expect we'll see 2.10 support anytime soon. This is blocked by scala.meta which uses a ton of macro annotations.

If we could get format region (e.g. by providing start/end positions) that would be even better.

That's not supported, yet. It's been on the roadmap since early days, I even have a prototype implementation that works for a lot of cases. I can't tell when I'll have time to properly complete it.

@olafurpg
Copy link
Member Author

BTW, I would love to have scalafmt support in ENSIME ^_^

However, scalafmt is a very different beast from scalariform. Reaching feature parity with scalariform is not on the roadmap.

@fommil
Copy link

fommil commented Sep 29, 2016

We won't be accessing it from the server, I was thinking tighter sbt integration was the way to go. From an end user perspective, it doesn't really matter, but it simplifies things a lot.

@olafurpg
Copy link
Member Author

Regressions are currently tested manually for non-trivial changes. Given that scalafmt output is quite stable now I propose to close this ticket.

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

No branches or pull requests

2 participants