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: Common stanzas #4751

Merged
merged 1 commit into from
Dec 13, 2017
Merged

RFC: Common stanzas #4751

merged 1 commit into from
Dec 13, 2017

Conversation

phadej
Copy link
Collaborator

@phadej phadej commented Sep 8, 2017

Common stanzas


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!

@mention-bot
Copy link

@phadej, thanks for your PR! By analyzing the history of the files in this pull request, we identified @Ericson2314, @ezyang and @jutaro to be potential reviewers.

@phadej phadej requested a review from hvr September 8, 2017 10:59
@phadej
Copy link
Collaborator Author

phadej commented Sep 8, 2017

Attempt on #2832

Copy link
Member

@23Skidoo 23Skidoo left a comment

Choose a reason for hiding this comment

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

Looks fine too, though we'll need a changelog note and some docs (same applies to the elif patch).

@phadej
Copy link
Collaborator Author

phadej commented Dec 12, 2017

I have redone the PR. Ping @23Skidoo @hvr.

There's internal note, I'll write a user guide section about common stanzas tomorrow if that behavior is ok.

Difference to previous is that you cannot say

-- not ok
library
  build-depends: foo
  include common-stanza

includes should be first in stanza:

-- ok
library
  include common-stanza
  build-depends: foo

@23Skidoo
Copy link
Member

23Skidoo commented Dec 12, 2017

@phadej This restriction seems reasonable to me, in fact I think that it improves readability.

@phadej
Copy link
Collaborator Author

phadej commented Dec 12, 2017

Two questions:

  • Should common stanzas be defined before use (currently: no. Done in separate pass)
  • Should common stanzas be able to reference other common stanzas? (currently: no)

Easy to implement are:

  • No/No: current
  • Yes/Yes: we can parse common stanzas as we go, in single pass. Parsing common stanza is not much different than parsing other stanzas
  • Yes/No: even simpler than above

Fourth variant is difficult, because with "Define everywhere + reference others (No/Yes):" we can write cycles:

common foo
  include bar

common bar
  include foo

@23Skidoo
Copy link
Member

I'd say that requiring common stanzas to be defined before use is fine (provided that the error message is helpful). Regarding inheritance -- do you want to allow only

common common-stanza
    include another-common-stanza
    ...

or also

common common-stanza
    include common-stanza-a
    include common-stanza-b
    ...

?

@phadej
Copy link
Collaborator Author

phadej commented Dec 12, 2017

You can include as many common stanzas as you like. There is no technical reason to limit that.

@phadej
Copy link
Collaborator Author

phadej commented Dec 12, 2017

To clarify the previous question, it can be posed also as Do we want to allow including stanzas from other common stanzas?. If yes, than we have to require define-before-use.

@23Skidoo
Copy link
Member

Do we want to allow including stanzas from other common stanzas?

IMO it can be useful, so I'm +1.

One question: since IIRC list fields have monoid semantics, should there be a way to explicitly set a list field to []? For example, IIUC,

common foo
    cpp-options: -foo

common bar
    cpp-options: -bar

common foobar
    include foo
    include bar

common barfoo
    include bar
    include foo

will result in foobar having cpp-options: -foo -bar, and barfoo having cpp-options: -bar -foo.

Should it be possible to do something like

common foo
    cpp-options: -foo

common bar
    include foo
    cpp-options := -bar

common baz
    include bar

and have baz's cpp-options be set to -bar only?

@phadej
Copy link
Collaborator Author

phadej commented Dec 12, 2017

@23Skidoo

Should it be possible to do something like

common bar
    include foo
    cpp-options := -bar

That's technically challenging because that alters Cabal's lexical syntax, which is very major change.
For that example one could factor common (pun-intended) parts of foo into quux and have

common quux
    ...

common foo
    include quux
    cpp-options: -foo

common bar
    include quux
    cpp-options := -bar

common baz
    include bar

@hvr
Copy link
Member

hvr commented Dec 12, 2017

One question: since IIRC list fields have monoid semantics, should there be a way to explicitly set a list field to []?

What about e.g.

common bar
    include foo
    !cpp-options: -bar

?

However, I feel like we can postpone this, and add it at some later spec-version incrementally, when have gained more experienced/feedback with the basic support. (But we can surely spend some thought on how we could do it at some point in the future of course; but I think we are pretty sure we know how to do the "MVP" version now?)

@23Skidoo
Copy link
Member

23Skidoo commented Dec 12, 2017

However, I feel like we can postpone this, and add it at some later spec-version incrementally, when have gained more experienced/feedback with the basic support.

Yeah, it's totally fine to postpone this. I like the !cpp-options: syntax proposal.

@phadej
Copy link
Collaborator Author

phadej commented Dec 12, 2017

Last minute changes, but with docs:

screenshot from 2017-12-12 18-09-28

@phadej phadej force-pushed the common-stanzas branch 4 times, most recently from f02f838 to 9d6f4da Compare December 12, 2017 16:41
- common stanzas can be include other common stanzas
- `import: name1, name2` to import multiple stanzas
- Parse common stanzas in the same pass with other sections.
- Common stanzas have to be defined before use.
- Also negative tests
- Terse documentation, let's improve it as questions are asked

- Edit gen-extra-source-files to include golden files
- Amend elif warning to mention cabal-version: 2.2
- In regression golden tests, include also warnings

Note: ATM the common stanzas are completely handled inside parser,
GenericPackageDescription doesn't know about them anymore.
That can be changed, but the the flattening of
GenericPackageDescription to PackageDescription may fail.
I don't want to do that refactor now.
@phadej phadej added this to the 2.2 milestone Dec 12, 2017
@phadej phadej merged commit b8c95ea into haskell:master Dec 13, 2017
@phadej phadej deleted the common-stanzas branch December 13, 2017 07:11
@gbaz
Copy link
Collaborator

gbaz commented Mar 23, 2018

do we need a docs tweak? from reddit: "Unless I'm mistaken, it appears that import has to be the first thing in a stanza, but this isn't mentioned anywhere."

(https://www.reddit.com/r/haskell/comments/85tuly/latest_hackage_deployment/dw4mf1j/)

@ekmett
Copy link
Member

ekmett commented Sep 4, 2018

I was just bitten by precisely the same thing @gbaz mentioned here.

@domenkozar
Copy link
Collaborator

Doc change: #5610

@andreasabel andreasabel added the Cabal: common stanza Concerning `common` stanzas in `.cabal` files label Jul 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Cabal: common stanza Concerning `common` stanzas in `.cabal` files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants