search

twitter / scalding

A Scala API for Cascading
http://twitter.com/scalding
Apache License 2.0
3.48k stars 703 forks source link

Migrate scalding from scalariform to scalafmt #1953

Closed ttim closed 2 years ago

ttim commented 2 years ago

Scala community migrated of scalariform towards scalafmt during last couple of years. In this PR I've replaced scalariform with scalafmt in Scalding, used scalafmt config from algebird, and reformatted scalding repo. I'm planning to follow up with another PR to enable scalafmt check on CI so it would be impossible to merge not formatted code.

tlazaro commented 2 years ago

Nice!

tlazaro commented 2 years ago

Are we sure this is based on the latest code changes we landed on develop? It's very hard to review the whole diff.

johnynek commented 2 years ago

looks like one of the job failed.

Also, I wonder, are we using scalding-quotation? I think that effort may have been abandoned and merged code that was never actually used.

ttim commented 2 years ago

Are we sure this is based on the latest code changes we landed on develop? It's very hard to review the whole diff.

Yes, I applied it on top of develop. But given build failures I decided to split this change into config & preparation, and actual reformatting diff which is going to be just reformatting (and making CI fail if something not formatted properly).

Now this change contains:

ttim commented 2 years ago

@johnynek

Also, I wonder, are we using scalding-quotation?

I don't think so. We should consider either adopting it or removing... cc/ @tlazaro https://github.com/twitter/scalding/pull/1763 here is change we never merged. Basically it adds more information about user facing API callsites to scalding internals.