search

IntersectMBO / cardano-api

Cardano API
Apache License 2.0
22 stars 20 forks source link

Adjustments to formatter #583

Closed palas closed 2 months ago

palas commented 2 months ago

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 https://github.com/IntersectMBO/cardano-api/pull/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

palas commented 2 months ago

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 https://github.com/IntersectMBO/cardano-api/pull/582#discussion_r1668232104:

  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.

carbolymer commented 2 months ago

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 commented 2 months ago

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. 😃