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

Remove unnecessary indentation from list comprehension #1119

Closed

Conversation

brandonchinn178
Copy link
Collaborator

@brandonchinn178 brandonchinn178 commented Jun 3, 2024

Resolves #966.

List comprehensions have an extra indentation because the current implementation treats comprehensions the same as a normal list, so it treats the piped section as being inside the list.

The bulk of the change is simply to only sitcc the list comprehension when s == S. This unindents list comprehensions in most circumstances. The remaining issue is list comprehensions on the RHS of a bind in a do-block; previously, all expressions in a do-block are marked as S, but the RHS of a bind does not require S. To fix this, I updated p_stmt' to take in BracketStyle so that BindStmt can explicitly switch it to N.

Semi-related to #1116, but an independent change. Additional context here: fourmolu/fourmolu#418 (comment)

Review instructions

Recommend reviewing commit-by-commit. There are a lot of commits, but they can broken down like this:

# Break out list comprehension rendering
54a95e1 Break out p_listComp (verbatim)
088cda6 Break out rendering body
afed56a Break out let definitions

# Inline functionality
ce25885 Inline brackets logic
1d30e96 Break out BracketStyle from render function

# Make the actual functional change
1a09212 Remove unnecessary indent for list comprehensions in do blocks
dbc476c Reformat codebase

Copy link
Member

@amesgen amesgen left a comment

Choose a reason for hiding this comment

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

Thanks!

This will change quite a lot of Ormolu-clean code in the wild (see eg for our hackageTests), but it is very innocent.

No strong opinion either way on the refactorings in the first few commits; based on your question/remark in one of the comments there, I even suggested a potentially different refactoring, see the corresponding review comment #1119 (comment).

src/Ormolu/Printer/Meat/Declaration/Value.hs Outdated Show resolved Hide resolved
src/Ormolu/Printer/Meat/Declaration/Value.hs Outdated Show resolved Hide resolved
src/Ormolu/Printer/Meat/Declaration/Value.hs Outdated Show resolved Hide resolved
src/Ormolu/Printer/Meat/Declaration/Value.hs Outdated Show resolved Hide resolved
src/Ormolu/Printer/Meat/Declaration/Value.hs Outdated Show resolved Hide resolved
@amesgen
Copy link
Member

amesgen commented Jun 6, 2024

Maybe stack this PR on top of #1121?

@brandonchinn178 brandonchinn178 force-pushed the bchinn-list-comp-do branch 2 times, most recently from 9c965bb to c04cd9d Compare June 6, 2024 23:11
Copy link
Member

@amesgen amesgen 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 from my side, I also updated the PR description to account for the commits that were moved to #1121.

@amesgen amesgen requested a review from mrkkrp June 7, 2024 10:26
@mrkkrp
Copy link
Member

mrkkrp commented Jun 12, 2024

Applied as 5c816de. Thanks!

@mrkkrp mrkkrp closed this Jun 12, 2024
@brandonchinn178
Copy link
Collaborator Author

just out of curiosity, is there a reason you didn't use github's squash and merge functionality?

@brandonchinn178 brandonchinn178 deleted the bchinn-list-comp-do branch June 12, 2024 17:03
@mrkkrp
Copy link
Member

mrkkrp commented Jun 12, 2024

I just don't have the habit of using that, so I am always a little bit unsure whether it will create a merge commit or not. I do not like having merge commits. I usually first do a rebase to prepare "pretty history" and then I do "rebase and merge". Here, I wanted to do some tweaks in the changelog (I think I added the link to the issue) and remembering that you don't like your branch to be modified I decided to apply to master like this. I think I could as well push a commit on top and then do "squash and merge".

@brandonchinn178
Copy link
Collaborator Author

Makes sense! If it's a small CHANGELOG commit, I'm fine with you pushing to my branch.

FYI you can go to Settings and disable merge commits when merging PRs. You should be able to force PRs to merge with Squash + Merge

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.

Unindent list comprehension
3 participants