Skip to content

Configurable newlines between declarations#48

Merged
georgefst merged 4 commits intofourmolu:masterfrom
wraithm:master
Oct 13, 2020
Merged

Configurable newlines between declarations#48
georgefst merged 4 commits intofourmolu:masterfrom
wraithm:master

Conversation

@wraithm
Copy link
Contributor

@wraithm wraithm commented Oct 9, 2020

The number of newlines between declarations can now be configured with 'newlines-between-decls' configuration option.

Closes #47

This does exactly what I want! It's idempotent as well. It defaults to exactly ormolu behavior. I tested it on our large industrial codebase. I don't see anything unexpected from my initial skim. If we merge this, I think we'd start using this at @bitnomial.

@wraithm
Copy link
Contributor Author

wraithm commented Oct 9, 2020

@georgefst @parsonsmatt What do you think?

@wraithm
Copy link
Contributor Author

wraithm commented Oct 9, 2020

Also, all tests pass.

@parsonsmatt
Copy link
Collaborator

I'm happy to support a style that you like!

The number of newlines between declarations can now be configured with
'newlines-between-decls' configuration option.
Copy link
Collaborator

@georgefst georgefst left a comment

Choose a reason for hiding this comment

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

This is great, and simpler than I expected. Just a few minor things.

@georgefst georgefst mentioned this pull request Oct 13, 2020
@georgefst
Copy link
Collaborator

georgefst commented Oct 13, 2020

Obviously there are no tests for what this option actually does (only that it has no effect when disabled), but that's really an issue with our test suite: #49

- Don't change upstream stuff in `newlineRaw`
- Don't need `declBreakpoint` because there will never be horizontal
  spaces between decls.
@wraithm
Copy link
Contributor Author

wraithm commented Oct 13, 2020

Okay @georgefst, I responded to all comments and pushed your requested changes 👍

@georgefst
Copy link
Collaborator

Ok @wraithm, I'll merge if you're happy with my new changes.

@wraithm
Copy link
Contributor Author

wraithm commented Oct 13, 2020

@georgefst I'm very happy. Go ahead!

@georgefst georgefst merged commit cce6ecd into fourmolu:master Oct 13, 2020
@wraithm
Copy link
Contributor Author

wraithm commented Oct 13, 2020

🎉 Thanks!

@wraithm
Copy link
Contributor Author

wraithm commented Oct 13, 2020

@georgefst Would you mind making a hackage publish at some point soon? We're gonna convert to fourmolu at @bitnomial asap.

@georgefst
Copy link
Collaborator

I suppose we could make a release with this and #44, which I'll hopefully get merged today.

I think PVP forces this to be a major release since it adds a record field (although I'd be surprised if Fourmolu were actually being used as a library anywhere other than in HLS, which I know wont be broken by this).

@georgefst
Copy link
Collaborator

@wraithm Done.

@wraithm
Copy link
Contributor Author

wraithm commented Oct 13, 2020

You the best. Thanks!

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.

Configurable number of lines between top-level definitions

3 participants