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

Router: use indent.{binPackXxx,afterInfix}Site #4057

Merged
merged 3 commits into from
Jun 17, 2024

Conversation

kitbellew
Copy link
Collaborator

They are needed to implement scala.js style.

@kitbellew kitbellew requested a review from tgodzik June 17, 2024 06:03
danglingParentheses.defnSite = false
binPack.defnSite = always
indent.defnSite = 2
indent.binPackDefnSite = 4
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does it need to be separate to indent.defnSite, that seems to over complicate things

Copy link
Collaborator Author

@kitbellew kitbellew Jun 17, 2024

Choose a reason for hiding this comment

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

how would you propose to do it? they have a requirement to use one indent when config style formatting is used, and a different one otherwise. which seems to mean one parameter is not enough...

Copy link
Contributor

Choose a reason for hiding this comment

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

It's probably not possible any other way, but I don't see how this kind of option is useful for anyone to have some many different indents

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you point me to the requirement, where it's defined?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nvm. I found it, but I am still not convinced it's more than just an OCD thing and doesn't really help in readability that much.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i think the reason it's useful to scala.js developers is that it visually distinguishes between clean config style formatting, and more readability-challenged binpack formatting. one of the reasons scala.js has not adopted scalafmt yet.

Copy link
Contributor

@tgodzik tgodzik left a comment

Choose a reason for hiding this comment

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

Looks good, I just always have doubts if we need to add more options to already an abundance of configuration

@kitbellew kitbellew merged commit a54770c into scalameta:master Jun 17, 2024
9 checks passed
@kitbellew kitbellew deleted the 4057 branch June 17, 2024 13:04
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

2 participants