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

RFC: Create cabal fix command #5741

Open
m-renaud opened this issue Nov 27, 2018 · 9 comments
Open

RFC: Create cabal fix command #5741

m-renaud opened this issue Nov 27, 2018 · 9 comments

Comments

@m-renaud
Copy link
Collaborator

m-renaud commented Nov 27, 2018

Overview

So, it became clear in #5734 that the purpose of cabal format is not, in fact, to format your cabal files, which I was sad to hear because I was looking for a tool to do that for a while. The main reason appears to be because it operates on a GenericPackageDescription which doesn't maintain source file location (or things like common stanzas).

I propose that we create a cabal fix command with the following behaviour:

  • Reformat the cabal files for your project, with sane defaults (but configurable, like indent offset)
  • If you have a Haskell code formatter specified in the config, re-format changed files.

Don't let great be the enemy of good

Yes, it would be possibly to build a full parser like exists for GPD, and also store source file locations, how things were populated, etc., but we could get a pretty long way with a much simpler and dumber formatter. Simple things like: remove trailing whitespace, always leave 1 space after the :, two blank lines between stanzas, always hang "repeated fields" and put one per line (build-depends, exposed-modules, etc). Maybe it can't handle everything from the beginning, but if it can successfully format the majority of folks cabal files then I think that's a win.

High Level Flow

  1. Update cabal check to return different error code on parse failure vs. warning.
  2. Add cabal fix, which makes sure the file parses first (via cabal check)
    • If parsing fails, ask the user to fix the issues first.
    • If parsing succeeds, continue on to re-formatting.
  3. Save a copy of the current cabal file (.cabal-fix/cabal-file.cabal.bak.n)
  4. Re-format the cabal file
    • Don’t try to be too smart
    • Can use a simplified package description (see below)
  5. Verify parsing of re-formatted cabal file succeeds:
    • If parsing fails, report failure and request a bug report be filed and revert file.
    • If parsing succeeds, then done.

Simplified Package Description

Since the majority of what we need to do doesn't care what an individual field or stanza represents, all we need to know is if the field is a single values field, a repeated field with optional commas, or a repeated field with required. This means we can use a much simpler package description and keep a list of which fields require different parsers/pretty printers. This should simplify the development greatly.

Support undo/redo

In case the formatter messes something up, there should be an easy way to revert it. One simple scheme is described below:

cabal fix --undo

This will replace cabal-file.cabal with .cabal-fix/cabal-file.cabal.bak.n and move the current cabal file to .cabal-fix/cabal-file.cabal.redo.n

cabal fix --redo

This will replace cabal-file.cabal with .cabal-fix/cabal-file.cabal.redo.n

Sample Formatted File

cabal-version: 2.4
name: myapp
version: 0.1.0.0
license: BSD-3-Clause
license-file: LICENSE
build-type: Simple
synopsis: My application synopsis. Should be short. One line.
description:
  My application description. This is probably kinda long so we hang it
  on the next line.


common common-deps
  default-language: Haskell2010
  ghc-options:
    -Wall
    -Wincomplete-uni-patterns
    -Wincomplete-record-updates
    -Wcompat
    -Widentities
    -Wredundant-constraints
    -fhide-source-paths
    -Wmissing-export-lists
    -Wpartial-fields
  build-depends:
    base >=4.12 && <4.13,


library
  import: common-deps
  exposed-modules:
    Myapp
  hs-source-dirs: src
  build-depends:
    containers ^>=0.6,


executable myapp
  import: common-deps
  main-is: Main.hs
  hs-source-dirs: app
  build-depends:
    myapp -any

Prior Art

http://hackage.haskell.org/package/stylish-cabal (just found this, will check it out)

Edit: This looks like a good start, I'm personally not a fan of the indentation style, but I'll start by poking around in the code there and see what I can do.

Implementation

I've been looking for little side project to write in Haskell for a while so I'd be willing to implement this.

/cc @phadej @vrom911

@gbaz
Copy link
Collaborator

gbaz commented Nov 27, 2018

@m-renaud see my comment on the other ticket. I think the amount of work here could profitably be put into resolving more issues with format and won'tfix was the wrong tag to give that ticket... I'd like to pursue further discussion there before continuing down this line.

@vrom911
Copy link
Member

vrom911 commented Nov 27, 2018

@m-renaud very thoughtful proposal. I generally also love the idea to have the formatter for such purposes. But probably I would need more from that, like alphabetic order for build-depends or modules, base/base-noprelude always on the first place, etc., so I'm not sure if it fits well into cabal itself, because the formatter either would have the large set of settings or it would be opinionated.

Details question: I don't really get why one could need redo command, I guess, it's more intuitive for the user to run cabal fix command again and this also won't let us format with a loss, for example, if the .cabal file was change between undo and redo commands.

But overall I like the idea, and would be happy to see at least light version of it here.

@benjaminweb
Copy link

I want to raise the attention for a recently released formatting tool ormolu.

What cargo fmt is for rust, could be cabal fmt for Haskell.
The actual formatter rustfmt could be ormolu.

How can ormolu be integrated into cabal?

Identified requirements to date are:

@phadej
Copy link
Collaborator

phadej commented Nov 19, 2019

This is issue is about formatting *.cabal files, not *.hs sources.


My opinion is that ormolu could depend on Cabal the library to parse the .cabal files, and then identify the sources (like sdist command does, all of that is few dozens lines of code).

Or to say it directly: we won't hard code any formatters, why ormolu and not say brittany?

@benjaminweb
Copy link

My opinion is that ormolu could depend on Cabal the library to parse the .cabal files, and then identify the sources (like sdist command does, all of that is few dozens lines of code).

Thanks for the idea!

@mrkkrp
Copy link

mrkkrp commented Nov 19, 2019

My opinion is that ormolu could depend on Cabal the library to parse the .cabal files, and then identify the sources (like sdist command does, all of that is few dozens lines of code).

Ormolu is build-system independent. It depends on GHC but it won't depend on Cabal. There won't be anything that goes beyond single-file processing, in particular there won't be anything that will have anything to do with Cabal files (parsing them, etc.). It is outside of the scope of the tool.

Or to say it directly: we won't hard code any formatters, why ormolu and not say brittany?

Sure. The way to do is to have some sort of plugin system for this.

@phadej
Copy link
Collaborator

phadej commented Nov 19, 2019

There's impedance mismatch in what tool writers want from build tools, e.g. hie-bios would be happy with bare information how to construct ghci command: or so I understood.

Please, if you want discuss Haskell code formatters, open a new issue. This issue is formatting *.cabal files.

I'm locking the discussion until tomorrow.

@haskell haskell locked as off-topic and limited conversation to collaborators Nov 19, 2019
@emilypi emilypi added the type: RFC Requests for Comment label Sep 9, 2021
@ffaf1
Copy link
Collaborator

ffaf1 commented Oct 6, 2023

Update cabal check to return different error code on parse failure vs. warning.

As now (b101f2d), 1 exit is for “Hackage will refuse the package”. I will see if there is a way to accomodate a “parse error” indication.

`cabal check` exits with 0 or 1. As now the `1` exit is slightly
stricter than “Hackage would reject this”, as even PackageDistSuspicious
 errors will trigger a `1` exit.

@geekosaur
Copy link
Collaborator

A useful and common (see for example grep(1)) convention is that exit 1 is for primary failure (here, would be rejected by Hackage) and higher exits are for failures that prevent determination of the primary status (for grep that means things like invalid regexes).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

9 participants