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

Adjustments to formatter #583

Merged
merged 5 commits into from
Jul 11, 2024
Merged

Adjustments to formatter #583

merged 5 commits into from
Jul 11, 2024

Conversation

palas
Copy link
Contributor

@palas palas commented Jul 8, 2024

Changelog

- description: |
    Further improvements to formatting and hooks
# uncomment types applicable to the change:
  type:
  - maintenance    # not directly related to the code

Context

This is a follow up PR for #582, and it aims to address some of the suggestions provided post-merge.

How to trust this PR

Probably check that the hook works for you. Also that you are happy with the changes and configuration, and that the CI passes.

Probably good idea to look at the commits separately.

Checklist

  • Commit sequence broadly makes sense and commits have useful messages
  • New tests are added if needed and existing tests are updated. See Running tests for more details
  • Self-reviewed the diff

@palas palas added enhancement New feature or request test Adding or reworking tests labels Jul 8, 2024
@palas palas self-assigned this Jul 8, 2024
@palas palas requested review from carbolymer and removed request for carbolymer July 8, 2024 23:35
@palas palas mentioned this pull request Jul 8, 2024
3 tasks
@palas palas force-pushed the adjustments-to-formatter branch 2 times, most recently from a19a48d to 1be7584 Compare July 8, 2024 23:39
Copy link
Contributor

@carbolymer carbolymer left a comment

Choose a reason for hiding this comment

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

I understand that the two formatters are needed for imports organization? That seems cumbersome (because we will have to deal with potential quirks of both of them) and will have poor code editor integration. Can we avoid that?

Continuing from #582 (comment):

  1. Regarding import grouping, I think this should be achievable using fourmolu custom grouping: https://fourmolu.github.io/config/import-grouping/
  2. I propose to enable ImportQualifiedPost GHC extension in cardano-api.cabal file globally, to improve imports readability - especially when qualified are mixed with non-qualified. Thoughts?
  3. We should mention usage of fourmolu in CONTRIBUTING.md. One additional small thing - would you mind moving CONTRIBUTING.md from cardano-cli to cardano-api repo, and then linking from cli to here?

@palas
Copy link
Contributor Author

palas commented Jul 9, 2024

I understand that the two formatters are needed for imports organization? That seems cumbersome (because we will have to deal with potential quirks of both of them) and will have poor code editor integration. Can we avoid that?

Continuing from #582 (comment):

  1. Regarding import grouping, I think this should be achievable using fourmolu custom grouping: https://fourmolu.github.io/config/import-grouping/

  2. I propose to enable ImportQualifiedPost GHC extension in cardano-api.cabal file globally, to improve imports readability - especially when qualified are mixed with non-qualified. Thoughts?

  3. We should mention usage of fourmolu in CONTRIBUTING.md. One additional small thing - would you mind moving CONTRIBUTING.md from cardano-cli to cardano-api repo, and then linking from cli to here?

The grouping feature doesn't seem to work in fourmolu, it says unreleased in the documentation, so I think it didn't make it to a release yet. stylish-haskell really only touched the imports, so I don't think it is going to bother us much, but it is up to you. I will check whether fourmolu respects the extension thing, but I am not sure it will be very convenient to have qualified at the end. I could make a wrapper script for fourmolu and stylish-haskell too, so it is a single command in the development shell. Also we are already using hlint too.

@palas palas force-pushed the adjustments-to-formatter branch from 1be7584 to 15c0891 Compare July 9, 2024 14:49
@palas palas force-pushed the adjustments-to-formatter branch from 15c0891 to f2363ec Compare July 9, 2024 14:52
@palas palas force-pushed the adjustments-to-formatter branch from f2363ec to 10c478e Compare July 9, 2024 16:18
@palas palas requested a review from carbolymer July 9, 2024 16:18
@carbolymer
Copy link
Contributor

The grouping feature doesn't seem to work in fourmolu, it says unreleased in the documentation, so I think it didn't make it to a release yet.

Oh you are right. It didn't make it to the latest release. Thanks for checking that.

@carbolymer
Copy link
Contributor

carbolymer commented Jul 10, 2024

I could make a wrapper script for fourmolu and stylish-haskell too, so it is a single command in the development shell.

I'm not 100% sold on using two formatters at the same time. We will have to deal with the potential issues of both, their two configuration formats, their two slightly different usages.

But if you see that's the best option we have at the moment, I'm ok with it 👌🏻. And having a formatting script executing both is then a must. 😃

scripts/devshell/format Outdated Show resolved Hide resolved
scripts/devshell/format Outdated Show resolved Hide resolved
scripts/devshell/format Outdated Show resolved Hide resolved
@palas palas requested a review from carbolymer July 11, 2024 01:46
@palas palas added this pull request to the merge queue Jul 11, 2024
Merged via the queue into main with commit 6dd96c6 Jul 11, 2024
24 checks passed
@palas palas deleted the adjustments-to-formatter branch July 11, 2024 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request test Adding or reworking tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants