upgrade scalafmt to 3.8.1

[oyvindberg]

#2215 won't build until there is a newer scalafmt, because of some quote/splice syntax.

I think most of the changes are for the worse - would you be willing to remove the align.preset = most from .scalafmt?
If you think it's a good idea I'll push a new PR with a commit doing just that.

[ghostdogpr]

We can try your suggestion. Also I don't know why it starts indenting with 4 characters, would be nice to change back to 2 to reduce the changes 🤔

[kyri-petrou]

I think most of the changes are for the worse - would you be willing to remove the align.preset = most from .scalafmt?

I have a feeling this will probably cause an extremely large number of changes. Maybe there is something in the config we can do to make the current scalafmt look more like before? without removing align.preset = most?

[kyri-petrou]

We can try your suggestion. Also I don't know why it starts indenting with 4 characters, would be nice to change back to 2 to reduce the changes 🤔

Actually I think it's still 2, it's just that in most of those cases it added 2 to the line above, so the nested ones look like now they have 4

[kyri-petrou]

I think we should add a .git-blame-ignore-revs file (see this post). Although, since we squash merge, I don't know whether it needs to be done manually in another PR or not 🤔

[ghostdogpr]

This change is also quite bad for readability :(

[oyvindberg]

In my experience, fixing the "ugly" cases like this is best done manually. For instance

  case _ => 
    val msg = """..."""
    sys.error(msg)

[ghostdogpr]

We can try your suggestion. Also I don't know why it starts indenting with 4 characters, would be nice to change back to 2 to reduce the changes 🤔

Actually I think it's still 2, it's just that in most of those cases it added 2 to the line above, so the nested ones look like now they have 4

I don't get what you mean. When it's nested there was already 2 spaces compared to the line above but now it's 4. Example:

[kyri-petrou]

I don't get what you mean. When it's nested there was already 2 spaces compared to the line above but now it's 4

Hmm I think in this case it might be a bug. It seems that it thinks the code in the parenthesis (starting with s""") should be on a newline. But then it doesn't format it in a new line and it looks weird 🤷

[oyvindberg]

I pushed a commit with the proposed change now.

Have a look and see if you like it, I much prefer it this way. In particular it's a plus that the examples no longer have long for comprehensions with huge amount of whitespace which makes them look unfamiliar.

I think we should add a .git-blame-ignore-revs file (see this post). Although, since we squash merge, I don't know whether it needs to be done manually in another PR or not 🤔

yes, It'll be easiest to add this file in a commit after the fact

[oyvindberg]

I see codegen tests must be updated as well, likely because they pick up the new scalafmt via BuildInfo (though I haven't looked deeply into it).

would you like to convert those to snapshot tests where we write to and compare contents in files instead, so we don't have to update these long strings manually? for local development we would always overwrite and then check into git, while in CI we can assert that the files be up to date.

[ghostdogpr]

It may be a matter of personal taste (and mostly habit) but I absolutely hate non-aligned for comprehensions 🤣 I find it extremely hard to parse.

[ghostdogpr]

I see codegen tests must be updated as well, likely because they pick up the new scalafmt via BuildInfo (though I haven't looked deeply into it).

would you like to convert those to snapshot tests where we write to and compare contents in files instead, so we don't have to update these long strings manually? for local development we would always overwrite and then check into git, while in CI we can assert that the files be up to date.

Yeah, why not. We had issues before because those strings were too long and had to find some tricky workarounds.

[oyvindberg]

I'll leave that decision to you, and will back out that last commit if you want it like it was 👍

It's rather confusing by the way that there is a scalafmt version declared in build.sbt and one in .scalafmt.conf. One was updated, and one was not. I'm not really sure what the interaction there is.

[ghostdogpr]

It's rather confusing by the way that there is a scalafmt version declared in build.sbt and one in .scalafmt.conf. One was updated, and one was not. I'm not really sure what the interaction there is.

The version in build.sbt is because the codegen depends on it for formatting the generated code, while .scalafmt.conf is just for caliban's code itself.

[kyri-petrou]

😨 I think it might be best that we put align = most back and let's stomach the few formatting issues with the new versions of scalafmt.

PS: I also hate non-aligned for-comprehensions. I don't understand why scalafmt doesn't align = and <- even with thw more preset - it does it only with most

[oyvindberg]

Ok, dropped that commit

[oyvindberg]

It's rather confusing by the way that there is a scalafmt version declared in build.sbt and one in .scalafmt.conf. One was updated, and one was not. I'm not really sure what the interaction there is.

The version in build.sbt is because the codegen depends on it for formatting the generated code, while .scalafmt.conf is just for caliban's code itself.

That makes sense. I'm wondering if scalafmt 3.8.0 reads .scalafmt.conf and downloads the old version and runs that. Otherwise we shouldn't see so much diff in the generated code

[ghostdogpr]

Does changing https://scalameta.org/scalafmt/docs/configuration.html#alignininterpolation to true make things better?

[oyvindberg]

Does changing https://scalameta.org/scalafmt/docs/configuration.html#alignininterpolation to true make things better?

no:

    def queryFromQueryParams(queryParams: QueryParams): DecodeResult[GraphQLRequest] =
      for {
        req <- requestCodec.decode(
                 s"""{"query":"","variables":${queryParams
                                                  .get("variables")
                                                  .getOrElse("null")},"extensions":${queryParams
                                                                                        .get("extensions")
                                                                                        .getOrElse("null")}}"""
               )

      } yield req.copy(query = queryParams.get("query"), operationName = queryParams.get("operationName"))

[ghostdogpr]

I'm okay with merging this (actually not all changes are bad, some are good) after solving the test failures.

[oyvindberg]

Sorry about the delay, I ran out of time for this. Pushed a commit with snapshot tests now, so hopefully we have a green build. I'll quote the commit message for some details:

Add SnapshotTest

This reads CI environment variable (same as sbt) to determine if we're running in CI.

This variable determines which behaviour the tests have

• If we're running in CI it checks that the contents of all the snapshot files are up to date
• Otherwise, it'll update the files with new contents.

The point is that a given change will showcase the effective results right in the PR, and otherwise be convenient to work with

It looks like circleci already sets the CI environment variable:

Using build environment variables:
  BASH_ENV=/tmp/.bash_env-6643432eabb43c0a727b7555-0-build
  CI=true
  CIRCLECI=true
  CIRCLE_BRANCH=pull/2216

[kyri-petrou]

omg CI's green 😃

[oyvindberg]

yeah that took a few attempts!

so order is 1) #2233 , 2) this, 3) #2215

[oyvindberg]

I rebased this this morning, so it should be good to go @ghostdogpr