cabal init -n: avoid extra blank lines #8292

ulysses4ever · 2022-07-16T01:11:35Z

cabal init -n's strategy is to create a list of field descriptions (the PrettyField type), which then gets transformed into a list of Blocks, which, finally, goes into a list of strings and those strings go right into the generated .cabal file.

Some fields may be deemed unnecessary depending on the init's flags (e.g. by default there's an executable field but no library field). Those fields are represented by the PrettyEmpty constructor of PrettyField. On the next stage, PrettyEmpty goes into a block with empty contents and no "margins" (cf. the Margin datatype). In theory, such empty blocks should turn into nothing in the output file. Unfortunately, the algorithm turning blocks into lines, flattenBlocks, is a bit tricky and does show sensitivity to empty blocks, producing spurious empty lines.

The simplest solution to this issue that I could think of is filtering out empty blocks before converting blocks to lines. This PR implements the idea.

Please include the following checklist in your PR:

Patches conform to the coding conventions.
Any changes that could be relevant to users have been recorded in the changelog.
The documentation has been updated, if necessary.

Please also shortly describe how you tested your change. Bonus points for added tests!

ulysses4ever · 2022-07-16T01:43:19Z

Hmm, funny: the failure doesn't reproduce locally...

Mikolaj · 2022-07-16T08:44:19Z

May be a filesystem issue? Let's re-run.

ulysses4ever · 2022-07-16T14:55:16Z

@emilypi @ptkato this PR seems to fix extra new lines in .cabal files generated by cabal init -n (#8236). Could you take a quick look and say if it makes sense to you? There are probably other ways to fight it (e.g. not generate empty blocks in the first place) but post-filtering looked like a more straightforward approach.

emilypi

One spurious change, the rest is sufficient. Nice job!

emilypi · 2022-07-16T16:07:41Z

Cabal-syntax/src/Distribution/Fields/Pretty.hs

+  { _beforeBlock   :: Margin
+  , _afterBlock    :: Margin
+  , _contentsBlock :: [String]
+  }


nice improvement!

emilypi · 2022-07-17T15:56:33Z

cabal-install/src/Distribution/Client/Init/FileCreators.hs

          message opts T.Warning "A .cabal file was found and not updated, if updating is desired please use the '--overwrite' option."

-      -- clear out last line for presentation.
-      T.putStrLn ""


why? This is purely for logging in terminal output. It has nothing to do with formatting files.

It is true that it's irrelevant to the contents of .cabal file. But it produces an extra empty line on the terminal (so you get two trailing empty lines), and this look completely unnecessary. So, I took liberty to remove it. In theory, this could go into a separate PR, but it feels like too much for such a simple change. Maybe a reasonable middle ground would be to put it into a separate commit, if you think it is worth it. If you firmly believe that a second trailing empty line should stay, I'm fine with reverting this change.

This is also not how all other commands behave and not how init used to behave: they all put one empty line, not two, in the end. E.g. cabal-install 3.6.2:

tmp/cabal-test-init/A via ❄️ impure (shell) ❯ cabal init -n -m Guessing dependencies... Generating LICENSE... Warning: unknown license type, you must put a copy in LICENSE yourself. Generating CHANGELOG.md... Generating app/Main.hs... Generating A.cabal... Warning: no synopsis given. You should edit the .cabal file and add one. You may want to edit the .cabal file and add a Description field. tmp/cabal-test-init/A via λ 9.0.2 via ❄️ impure (shell) ❯ cabal build Resolving dependencies... Build profile: -w ghc-9.0.2 -O1 In order, the following will be built (use -v for more details): - A-0.1.0.0 (exe:A) (first run) Configuring executable 'A' for A-0.1.0.0.. Preprocessing executable 'A' for A-0.1.0.0.. Building executable 'A' for A-0.1.0.0.. [1 of 1] Compiling Main ( app/Main.hs, /home/artem/tmp/cabal-test-init/A/dist-newstyle/build/x86_64-linux/ghc-9.0.2/A-0.1.0.0/x/A/build/A/A-tmp/Main.o ) Linking /home/artem/tmp/cabal-test-init/A/dist-newstyle/build/x86_64-linux/ghc-9.0.2/A-0.1.0.0/x/A/build/A/A ... tmp/cabal-test-init/A via λ 9.0.2 via ❄️ impure (shell) took 3s ❯

For comparison, the current init with two trailing empty lines:

tmp/cabal-test-init/A via ❄️ impure (shell) ❯ ~/bin/cabal-head init -n -m Warning: this is a debug build with assertions enabled. [Log] Guessing dependencies... [Log] Using cabal specification: 3.6 [Warning] unknown license type, you must put a copy in LICENSE yourself. [Log] Creating fresh file CHANGELOG.md... [Log] Creating fresh directory ./app... [Log] Creating fresh file app/Main.hs... [Log] Creating fresh file A.cabal... [Warning] No synopsis given. You should edit the .cabal file and add one. [Info] You may want to edit the .cabal file and add a Description field. tmp/cabal-test-init/A via λ 9.0.2 via ❄️ impure (shell) took 2s ❯

But where is that second newline coming from? It seems to be a function of your terminal

tmp/cabal-test-init/A via λ 9.0.2 via ❄️ impure (shell)

Is not trailing output in mine:

[1 of 1] Compiling Main ( main/Main.hs, /Users/emilypillmore/haskell/cabal/dist-newstyle/build/aarch64-osx/ghc-8.10.7/cabal-install-3.9.0.0/x/cabal/build/cabal/cabal-tmp/Main.o ) Linking /Users/emilypillmore/haskell/cabal/dist-newstyle/build/aarch64-osx/ghc-8.10.7/cabal-install-3.9.0.0/x/cabal/build/cabal/cabal ... Warning: this is a debug build with assertions enabled. [Log] Guessing dependencies... [Log] Using cabal specification: 3.6 [Warning] unknown license type, you must put a copy in LICENSE yourself. [Log] Creating fresh file CHANGELOG.md... [Log] Creating fresh directory ./app... [Log] Creating fresh file app/Main.hs... [Log] Creating fresh file tmp.cabal... [Warning] No synopsis given. You should edit the .cabal file and add one. [Info] You may want to edit the .cabal file and add a Description field. λ П tmp → λ git t8236* →

So it seems to me that you're using a nonstandard terminal here that appends data in the form of a newline and a message about what shell you're using. I'd say that's not something I'd care to support.

On your end, do you see that the current init leaves more trailing vertical space than init from any past release and also more than build or any other command? If yes, than why would we want having inconsistency between commands?

(By current I mean master).

cabal build:

λ П tmp → λ git t8236* → cabal build cabal: No targets given and there is no package in the current directory. Use the target 'all' for all packages in the project or specify packages or components by name or location. See 'cabal build --help' for more details on target options. λ П tmp → λ git t8236* →

cabal init -n -m :

[1 of 1] Compiling Main ( main/Main.hs, /Users/emilypillmore/haskell/cabal/dist-newstyle/build/aarch64-osx/ghc-8.10.7/cabal-install-3.9.0.0/x/cabal/noopt/build/cabal/cabal-tmp/Main.o ) Linking /Users/emilypillmore/haskell/cabal/dist-newstyle/build/aarch64-osx/ghc-8.10.7/cabal-install-3.9.0.0/x/cabal/noopt/build/cabal/cabal ... Warning: this is a debug build of cabal-install with assertions enabled. [Log] Guessing dependencies... [Log] Using cabal specification: 3.6 [Warning] unknown license type, you must put a copy in LICENSE yourself. [Log] Creating fresh file CHANGELOG.md... [Log] Creating fresh directory ./app... [Log] Creating fresh file app/Main.hs... [Log] Creating fresh file tmp.cabal... [Warning] No synopsis given. You should edit the .cabal file and add one. [Info] You may want to edit the .cabal file and add a Description field. λ П tmp → λ git master* →

cabal update:

λ П tmp → λ git master* → cabal run exe:cabal -O0 -- update Up to date Warning: this is a debug build of cabal-install with assertions enabled. Downloading the latest package list from hackage.haskell.org Package list of hackage.haskell.org has been updated. The index-state is set to 2022-07-17T19:36:44Z. To revert to previous state run: cabal v2-update 'hackage.haskell.org,2022-07-05T16:43:58Z' λ П tmp → λ git master* →

cabal unpack:

λ П tmp → λ git master* → cabal unpack foo Downloading foo-1.0 Downloaded foo-1.0 Unpacking to foo-1.0/ λ П tmp → λ git master* → cd foo-1.0 λ П foo-1.0 → λ git master* → cabal sdist foo.cabal cabal: Unknown target 'foo.cabal'. There is no component 'foo.cabal'. The project has no package .cabal file 'foo.cabal'. λ П foo-1.0 → λ git master* →

It seems to me that the single-line space clearing done after the command output is finished in terminal is currently standard in master for a few commands, including init. Its removal introduces inconsistency at least in this pr. However, functions like sdist introduce 2 newlines upon failure, and others none at all upon success. I think this warrants some discussion.

Fair, thank you for experimenting. I just reverted this change.

ulysses4ever · 2022-07-17T16:16:11Z

I updated the description, which now should be more helpful to potential reviewers.

ulysses4ever · 2022-07-18T11:11:01Z

@mergify backport 3.8

mergify · 2022-07-18T11:11:07Z

backport 3.8

🟠 Waiting for conditions to match

merged [:pushpin: backport requirement]

mergify · 2022-07-18T13:09:28Z

backport 3.8

✅ Backports have been created

#8298 cabal init -n: avoid extra blank lines (backport #8292) has been created for branch 3.8

cabal init -n: avoid extra blank lines (backport #8292)

ulysses4ever force-pushed the t8236 branch from f2e4181 to c7a8512 Compare July 16, 2022 01:13

ulysses4ever added attention: needs-help Help wanted with this issue/PR and removed attention: needs-help Help wanted with this issue/PR labels Jul 16, 2022

ulysses4ever force-pushed the t8236 branch 2 times, most recently from 914f48f to e0507dd Compare July 16, 2022 14:25

ulysses4ever marked this pull request as ready for review July 16, 2022 14:28

ulysses4ever added the attention: needs-review label Jul 16, 2022

ulysses4ever force-pushed the t8236 branch 2 times, most recently from ba6a6f0 to 53a421c Compare July 16, 2022 15:04

ulysses4ever added the attention: needs-backport 3.8 label Jul 17, 2022

ulysses4ever mentioned this pull request Jul 17, 2022

cabal init -n sometimes puts two newlines instead of one in .cabal file #8236

Closed

emilypi requested changes Jul 17, 2022

View reviewed changes

ulysses4ever force-pushed the t8236 branch from 53a421c to 6844e5d Compare July 17, 2022 20:54

emilypi approved these changes Jul 17, 2022

View reviewed changes

Mikolaj approved these changes Jul 18, 2022

View reviewed changes

ulysses4ever added the merge me Tell Mergify Bot to merge label Jul 18, 2022

cabal init -n: avoid extra blank lines (fix #8236)

97f6d96

ulysses4ever force-pushed the t8236 branch from 6844e5d to 97f6d96 Compare July 18, 2022 11:11

mergify bot merged commit 293bcd9 into master Jul 18, 2022

mergify bot mentioned this pull request Jul 18, 2022

cabal init -n: avoid extra blank lines (backport #8292) #8298

Merged

fgaz deleted the t8236 branch July 18, 2022 14:10

mergify bot added a commit that referenced this pull request Jul 18, 2022

Merge pull request #8298 from haskell/mergify/bp/3.8/pr-8292

470b4fe

cabal init -n: avoid extra blank lines (backport #8292)

Mikolaj removed the attention: needs-backport 3.8 label Jul 19, 2022

Mikolaj mentioned this pull request Jul 19, 2022

Standardize Command Output Logging Formats #8306

Open

cabal init -n: avoid extra blank lines #8292

cabal init -n: avoid extra blank lines #8292

Uh oh!

Conversation

ulysses4ever commented Jul 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ulysses4ever commented Jul 16, 2022

Uh oh!

Mikolaj commented Jul 16, 2022

Uh oh!

ulysses4ever commented Jul 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

emilypi left a comment

Choose a reason for hiding this comment

Uh oh!

emilypi Jul 16, 2022

Choose a reason for hiding this comment

Uh oh!

emilypi Jul 17, 2022

Choose a reason for hiding this comment

Uh oh!

ulysses4ever Jul 17, 2022

Choose a reason for hiding this comment

Uh oh!

emilypi Jul 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emilypi Jul 17, 2022

Choose a reason for hiding this comment

Uh oh!

ulysses4ever Jul 17, 2022

Choose a reason for hiding this comment

Uh oh!

ulysses4ever Jul 17, 2022

Choose a reason for hiding this comment

Uh oh!

emilypi Jul 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emilypi Jul 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ulysses4ever Jul 17, 2022

Choose a reason for hiding this comment

Uh oh!

ulysses4ever commented Jul 17, 2022

Uh oh!

ulysses4ever commented Jul 18, 2022

Uh oh!

mergify bot commented Jul 18, 2022

🟠 Waiting for conditions to match

Uh oh!

mergify bot commented Jul 18, 2022

✅ Backports have been created

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ulysses4ever commented Jul 16, 2022 •

edited

Loading

ulysses4ever commented Jul 16, 2022 •

edited

Loading

emilypi Jul 17, 2022 •

edited

Loading

emilypi Jul 17, 2022 •

edited

Loading

emilypi Jul 17, 2022 •

edited

Loading