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

Make a trailing colon for stanzas a parse failure #10525

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

9999years
Copy link
Collaborator

@9999years 9999years commented Nov 4, 2024

Here's a mistake I make semi-regularly:

source-repository-package:
    type: git
    location: https://github.com/parsonsmatt/foundation
    tag: 688c32ccd9a951bc96dd09423a6e6684f091d510
    subdir: basement
    subdir: foundation

Cabal treats this as a warning, so it prints:

Warning: cabal.project: Unrecognized field
'source-repository-package' on line 52

This is fine (if you already know the mistake you've made, at least!), but it's very easy to miss amidst lots of output.

I often re-run cabal when I see a ton of output to attempt to get a smaller error message. (Usually it works and I get an error message that's got less "compiling module such and such" noise in it.) However, re-running cabal will discard this warning entirely!

Let's make it a hard error instead. This is a backwards-compatibility break.

Now, it prints this error:

Error: [Cabal-7090]
Error parsing project file cabal.project:52:
'source-repository-package' is a stanza, not a field. Remove the trailing ':' to parse a stanza.
  • Still needs tests!

Here's a mistake I make semi-regularly:

    source-repository-package:
        type: git
        location: https://github.com/parsonsmatt/foundation
        tag: 688c32ccd9a951bc96dd09423a6e6684f091d510
        subdir: basement
        subdir: foundation

Cabal treats this as a warning, so it prints:

    Warning: cabal.project: Unrecognized field
    'source-repository-package' on line 52

This is fine (if you already know the mistake you've made, at least!),
but it's very easy to miss amidst lots of output.

I often re-run `cabal` when I see a ton of output to attempt to get a
smaller error message. (Usually it works and I get an error message
that's got less "compiling module such and such" noise in it.) However,
re-running `cabal` will discard this warning entirely!

Let's make it a hard error instead. This is a backwards-compatibility
break.
@9999years 9999years marked this pull request as ready for review November 18, 2024 23:41
@9999years
Copy link
Collaborator Author

9999years commented Nov 18, 2024

Added a test case and a release note, ready for review!

@Mikolaj
Copy link
Member

Mikolaj commented Nov 20, 2024

I always forget: cabal.project format is not guarded under the cabal-version, only the .cabal file format is, right?

@philderbeast
Copy link
Collaborator

Yes @Mikolaj, there's no cabal versioning in project files, neither a version stamp nor a conditional check for cabal's version, #8933 and #10386.

@Mikolaj
Copy link
Member

Mikolaj commented Nov 20, 2024

@philderbeast: thank you and these are indeed important tickets. So, this breaking change seems fine to be included in the next major version. I don't think it's breaking a useful enough behaviour that an advance warning or deprecation period is needed.

Copy link
Member

@Mikolaj Mikolaj left a comment

Choose a reason for hiding this comment

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

LGTM

@9999years 9999years added the merge me Tell Mergify Bot to merge label Nov 20, 2024
@mergify mergify bot added the ready and waiting Mergify is waiting out the cooldown period label Nov 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge me Tell Mergify Bot to merge ready and waiting Mergify is waiting out the cooldown period
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants