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

Migrate scalding from scalariform to scalafmt #1953

Merged
merged 7 commits into from Sep 27, 2021

Conversation

ttim
Copy link
Collaborator

@ttim ttim commented Sep 24, 2021

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
Copy link
Contributor

tlazaro commented Sep 24, 2021

Nice!

@tlazaro
Copy link
Contributor

tlazaro commented Sep 24, 2021

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
Copy link
Collaborator

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
Copy link
Collaborator Author

ttim commented Sep 25, 2021

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:

  • removal of scalariform
  • commented travis ci config to check for code formatting
  • removal of permgen size from our tests, this was failing running tests locally with modern JDK
  • reformat of TextMacroTest and TestJobsWithDescriptions because their reformat breaks tests

@ttim
Copy link
Collaborator Author

ttim commented Sep 25, 2021

@johnynek

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

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

@tlazaro tlazaro merged commit 07dd886 into twitter:develop Sep 27, 2021
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.

None yet

3 participants