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

Change scalafmt.conf #1133

Closed
wants to merge 1 commit into from
Closed

Change scalafmt.conf #1133

wants to merge 1 commit into from

Conversation

toddburnside
Copy link
Contributor

Use file from lucuma-ui

Use file from lucuma-ui
@bundlemon
Copy link

bundlemon bot commented Sep 28, 2021

BundleMon

Unchanged files (4)
Status Path Size Limits
index.(hash).js
9.5MB -
vendor.(hash).js
1.5MB -
index.(hash).css
767.2KB -
internal-0.(hash).js
619.41KB -

No change in files bundle size

Final result: ✅

@github-actions
Copy link

Visit the preview URL for this PR (updated for commit cdf8e09):

https://explore-gemini--pr1133-change-scalafmt-conf-h9j5el8j.web.app

(expires Tue, 05 Oct 2021 22:11:46 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

@rpiaggio
Copy link
Contributor

I missed the comment in lucuma-ui and I see now that we lost aligning =>s in match/cases and <- in fors. Or there seems to be a low threshold to keep them aligned. It'd be cool to just get those back.

@toddburnside
Copy link
Contributor Author

I missed the comment in lucuma-ui and I see now that we lost aligning =>s in match/cases and <- in fors. Or there seems to be a low threshold to keep them aligned. It'd be cool to just get those back.

Yeah. It's a bit odd. For example, in ConnectionsStatus.scala some, but not all of the => are aligned.

Copy link
Contributor

@cquiroz cquiroz left a comment

Choose a reason for hiding this comment

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

LGTM

@toddburnside
Copy link
Contributor Author

toddburnside commented Sep 29, 2021

Interestingly, it only seems to be possible to get the alignment of the => and <- with align.preset = most, not with any of the individual settings. Of course, that also brings back the aligned =, even for definitions separated by new lines.

Also, I like the align.arrowEnumeratorGenerator = true setting, which gives

image

instead of

image

This is something we had previously with the most preset, but can also be set independently.

I wonder if we should wait for the fix to scalameta/scalafmt#2775 in scalameta/scalafmt#2753 and go back to the most preset? The PR has been merged, and they seem to be making a new release every few hours.

@cquiroz
Copy link
Contributor

cquiroz commented Sep 29, 2021

I also prefer the alignment inside the for loops

We can certainly wait for upstream fixes

@toddburnside
Copy link
Contributor Author

I also prefer the alignment inside the for loops

We can certainly wait for upstream fixes

The failure to sometimes not align the => and <- had to do with multiline statements. align.multiline = true needed to be set, but that also caused the extreme alignment of the = even across spaces. There have been 2 PRs merged to fix the multiline issues.

Unless someone objects, I'll close this PR and see what happens when a new version of scalafmt-core is released. I would like to compare what happens with align.preset = most vs what is in this PR with the addition of align.multiline = true and align.arrowEnumeratorGenerator = true.

@toddburnside
Copy link
Contributor Author

toddburnside commented Sep 29, 2021

Just a note for future work. If we use align.preset = most I think we can get rid of all (or at least most) of the align.tokens settings as they are included in most.

One reason to favor setting the tokens explicitly vs the most preset is to not get surprised by future additions to the most preset. On the other hand, I guess using most would alert us to any new alignment options, desirable or not.

@toddburnside
Copy link
Contributor Author

Waiting on new version of scalafmt.

@toddburnside toddburnside deleted the change-scalafmt-config branch October 20, 2021 15:55
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