Make cabal init conform with `--no-comments` flag for commented fields #7770

ptkato · 2021-10-23T19:58:38Z

This PR fixes #7769.

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.

Mikolaj · 2021-10-23T20:21:16Z

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

+    | fieldContents == empty && (hasNoComments || isMinimal) =
+        fieldSEmptyContents NoComment
+    | fieldContents == empty =
+        fieldSEmptyContents $ commentPositionFor fieldName fieldComments
+
+      -- If the "--no-comments" or "--minimal" flag is set, strip comments.
+    | hasNoComments || isMinimal =
+        fieldSWithContents NoComment
+
    | otherwise =
-        -- If the "--no-comments" or "--minimal" flag is set, strip comments.
-        let comments
-              | isMinimal = []
-              | hasNoComments = []
-              | otherwise = fieldComments
-
-        -- If the "--minimal" flag is set, strip comments.
-        in fieldSWithContents comments
+        fieldSWithContents $ commentPositionFor fieldName fieldComments


Can this be written more succinctly?

Could you elaborate?

I didn't propose anything, just made a gut feeling suggestion. But if you'd like my help, let me think: wouldn't the following guards suffice: hasNoComments || isMinimal, otherwise. And if so, how come so much code is unneeded? Are we missing something?

oh, I just swapped things around, to make it more uniform, that's all.

Actually, my bad, I didn't notice that fieldSEmptyContents and fieldSWithContents are different functions. So the only simplification could be that fieldContents == empty decides between the two functions and hasNoComments || isMinimal between their arguments. But that's not necessarily more readable than separate four cases from multiplying the pair of two cases.

Well, let's try:

(if fieldContents == empty then fieldSEmptyContents else fieldSWithContents) $ if hasNoComments || isMinimal then NoComment else commentPositionFor fieldName fieldComments

You be the judge.

It's straightforwarder, but are you sure it's more readable?

No, I don't think so. It makes it harder to mix up the two functions that I mixed up and makes it clearer which bits determine which outcome. However, it's overall much harder to read and it's an eureka moment to even discover this is four-branch conditional in disguise. As I said, you be the judge --- it's your PR.

I think using a subexpression and guards make it more readable (and continue being more succint):

fieldD fieldName fieldContents fieldComments includeField opts | hasNoComments || isMinimal = contents NoComment | otherwise = contents $ commentPositionFor fieldName fieldComments where contents | fieldContents == empty = fieldSEmptyContents | otherwise = fieldSWithContents

But being shorter is not the main goal imo but no repeat conditions, as you will have to remember update 2 call sites if they change. Boolean blindness will make types not help with that.

It is a sound strategy, I'll be using that.

Mikolaj

LGTM

jneira

LGTM

jneira · 2021-10-25T09:01:13Z

cabal-install/tests/fixtures/init/golden/exe/exe.golden

@@ -1,11 +1,7 @@
 executable y


I've observed comments are being removed from test cases with -no-comment suffix (so makes total sense) but also from test cases without that suffix, is that on purpose? i suppose we have still test cases with comments, should we suffix these cases with -no-comment as well to be consistent?

It is on purpose, they're set to no comments, see here:

cabal/cabal-install/tests/UnitTests/Distribution/Client/Init/Golden.hs

Line 115 in e74a53b

let opts = WriteOpts False False True v pkgDir Executable pkgName defaultCabalVersion

Well i think if all cases without comments had have the -no-comment suffix maybe it would be more evident that something was wrong with the test cases themselves, as they had comments!

Here we go, now the golden files have more meaningful names.

Great, thanks for take in account our feedback 👍

jneira

LGTM

Mikolaj · 2021-10-26T20:55:33Z

@andreasabel: does the failure in docs/readthedocs.org:cabal job tell you anything? We are now waiting for your docs test to pass (or not) to compare...

andreasabel · 2021-10-27T07:16:14Z

@andreasabel: does the failure in docs/readthedocs.org:cabal job tell you anything? We are now waiting for your docs test to pass (or not) to compare...

Er, no, my docs test, named "Users guide" did not run on this PR, as it is only triggered by changes under /doc/ (see below).

This is the "Read the Docs" CI that is failing. I did not contribute it.
I looked at the error it reports, but it does not ring a bell.

cabal/.github/workflows/users-guide.yml

Lines 3 to 23 in e74a53b

    
           name: Users guide 
        
           on: 
        
             push: 
        
               branches: 
        
               - master 
        
               paths: 
        
               - 'doc/requirements.txt' 
        
               - 'doc/*.inc' 
        
               - 'doc/*.py' 
        
               - 'doc/*.rst' 
        
               - 'doc/**/*.json' 
        
               - '.github/workflows/users-guide.yml' 
        
             pull_request: 
        
               paths: 
        
               - 'doc/requirements.txt' 
        
               - 'doc/*.inc' 
        
               - 'doc/*.py' 
        
               - 'doc/*.rst' 
        
               - 'doc/**/*.json' 
        
               - '.github/workflows/users-guide.yml'

andreasabel · 2021-10-27T07:29:21Z

I tried to get more information on the docs/readthedocs.org:cabal workflow, but the relevant discussion ends without a resolution, see #7647 (comment).

Mikolaj · 2021-10-27T08:30:09Z

@andreasabel: thank you, in that case, let me mark your test, again, as not mandatory in Settings/

fgaz · 2021-10-27T08:30:15Z

I triggered a readthedocs build on master and it failed, so as expected the problem isn't introduced with this pr

andreasabel · 2021-10-27T09:51:08Z

@Mikolaj

@andreasabel: thank you, in that case, let me mark your test, again, as not mandatory in Settings/

Ah, so the PR was waiting for my test, which did not run because it is not triggered?
This is not visible from my POV, the list has no entry for "Users guide", and also I have no access to the Settings.

Of course, if the semantics of "mandatory" is "needs to run, and needs to succeed" (Run & Succeed) rather than "if it runs it should succeed" (Run => Succeed), then my test should not be "mandatory".

Mikolaj · 2021-10-27T09:59:11Z

Ah, so the PR was waiting for my test, which did not run because it is not triggered?

That's only because I force-added it to Settings/ after asking around on #hackage.

This is not visible from my POV, the list has no entry for "Users guide",

That's, because I've now removed it from Settings/.

Of course, if the semantics of "mandatory" is "needs to run, and needs to succeed" (Run & Succeed) rather than "if it runs it should succeed" (Run => Succeed), then my test should not be "mandatory".

Sadly, that's the semantics, so it was waiting for your test when it was marked in Settings for a moment (in a failed effort to check if it fails similarly to readthedocs test).

andreasabel · 2021-10-27T10:14:16Z

@fgaz wrote:

I triggered a readthedocs build on master and it failed, so as expected the problem isn't introduced with this pr

Also, locally the docs build fine with make users-guide.

A documentation concerning the offending section_self_link is here:
https://docutils.sourceforge.io/docs/user/config.html#section-self-link
It says:

New in Docutils 0.18.

Locally, I get 0.16:

docutils>=0.12 in ./.python-sphinx-virtualenv/lib/python3.9/site-packages (from sphinx==3.1.*->-r doc/requirements.txt (line 1)) (0.16)

Remotely (on rtd), 0.18 is used:

Requirement already satisfied: docutils>=0.12 in /home/docs/checkouts/readthedocs.org/user_builds/cabal/envs/7770/lib/python3.7/site-packages (from sphinx==3.1.*->-r doc/requirements.txt (line 1)) (0.18)

That can make the difference.
Likely, our requirements.txt is not watertight, so we might get packages that do not work together. (For developers of cabal, this is quite ironical, hahaha!)

Mikolaj · 2021-10-27T11:00:41Z

@andreasabel: thank you for diagnosing the problem and opening a new issue.

@ptkato: in that case, we are making ourselves scarce from your PR. Given that all other jobs passed fine, let me merge manually.

Mikolaj · 2021-10-27T11:00:59Z

@Mergifyio rebase

mergify · 2021-10-27T11:01:19Z

rebase

☑️ Nothing to do

-closed [:pushpin: rebase requirement]
#commits-behind>0 [:pushpin: rebase requirement]

ptkato added the attention: needs-review label Oct 23, 2021

Mikolaj reviewed Oct 23, 2021

View reviewed changes

Mikolaj approved these changes Oct 25, 2021

View reviewed changes

jneira approved these changes Oct 25, 2021

View reviewed changes

jneira reviewed Oct 25, 2021

View reviewed changes

ptkato force-pushed the cabal-init-commented-comments branch 2 times, most recently from 6d1a4aa to e211005 Compare October 26, 2021 18:44

ptkato added 2 commits October 26, 2021 16:59

Make cabal init conform with --no-comments flag for commented fields

d1736bd

Adapting tests' outputs and fixing naming

96375e5

ptkato force-pushed the cabal-init-commented-comments branch from e211005 to 96375e5 Compare October 26, 2021 20:01

jneira approved these changes Oct 26, 2021

View reviewed changes

andreasabel added cabal-install: cmd/init and removed attention: needs-review labels Oct 27, 2021

andreasabel mentioned this pull request Oct 27, 2021

Fix documentation build on readthedocs #7777

Closed

Mikolaj merged commit 41ed090 into haskell:master Oct 27, 2021

Make cabal init conform with --no-comments flag for commented fields #7770

Make cabal init conform with --no-comments flag for commented fields #7770

Uh oh!

Conversation

ptkato commented Oct 23, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mikolaj Oct 23, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mikolaj Oct 23, 2021 • 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!

jneira Oct 25, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

Mikolaj left a comment

Choose a reason for hiding this comment

Uh oh!

jneira left a comment

Choose a reason for hiding this comment

Uh oh!

jneira Oct 25, 2021 • 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!

ptkato Oct 26, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jneira left a comment

Choose a reason for hiding this comment

Uh oh!

Mikolaj commented Oct 26, 2021

Uh oh!

andreasabel commented Oct 27, 2021

Uh oh!

andreasabel commented Oct 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Mikolaj commented Oct 27, 2021

Uh oh!

fgaz commented Oct 27, 2021

Uh oh!

andreasabel commented Oct 27, 2021

Uh oh!

Mikolaj commented Oct 27, 2021

Uh oh!

andreasabel commented Oct 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Mikolaj commented Oct 27, 2021

Uh oh!

Mikolaj commented Oct 27, 2021

Uh oh!

mergify bot commented Oct 27, 2021

☑️ Nothing to do

Uh oh!

Make cabal init conform with `--no-comments` flag for commented fields #7770

Make cabal init conform with `--no-comments` flag for commented fields #7770

Mikolaj Oct 23, 2021 •

edited

Loading

Mikolaj Oct 23, 2021 •

edited

Loading

jneira Oct 25, 2021 •

edited

Loading

jneira Oct 25, 2021 •

edited

Loading

ptkato Oct 26, 2021 •

edited

Loading

andreasabel commented Oct 27, 2021 •

edited

Loading

andreasabel commented Oct 27, 2021 •

edited

Loading