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

Provide more context on Cabal file parsing errors #999

Merged
merged 1 commit into from Mar 31, 2023

Conversation

zzantares
Copy link
Contributor

When ormolu fails to parse the Cabal file the user doesn't know what is the actual error as only the cabal filepath is reported. This makes the necessary changes to actually show what errors the cabal file parser encountered.

Before:

  Parsing this .cabal file failed:
    /path/to/file.cabal

After:

  Parsing this .cabal file failed:
    /path/to/file.cabal:0:0: Unsupported cabal-version 3.8. See https://github.com/haskell/cabal/issues/4899.

Note: Removing the Eq instance on OrmoluException was the easy way to not
having to implement an orphan Eq instance on PError. This does not have
any impact elsewhere as we're actually not making use of the Eq instance
on this exception type.

@mrkkrp
Copy link
Member

mrkkrp commented Mar 13, 2023

As a consequence of c81320d, this actually constitutes now a breaking change 🤔

Copy link
Member

@amesgen amesgen left a comment

Choose a reason for hiding this comment

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

As a consequence of c81320d, this actually constitutes now a breaking change 🤔

Yeah, maybe the breaking change becomes more "justifiable" when we also add the error code to OrmoluException in #983:

ormolu/src/Ormolu/Parser.hs

Lines 115 to 117 in 65539b9

codeMsg = case diagnosticCode err of
Just code -> "[" <> showOutputable code <> "] "
Nothing -> ""

An option to prevent this in the future would be ofc to make OrmoluException opaque as most users probably just want to print it.

If we really want to, we can probably merge this and still maintain compatibility via some PatternSynonym dances (not sure if worth it).


In general, I think this change looks good, I think it is mostly useful when Ormolu uses a different cabal version than the installed cabal binary (as otherwise, you get the same error message when invoking cabal).

src/Ormolu/Exception.hs Outdated Show resolved Hide resolved
src/Ormolu/Utils/Cabal.hs Outdated Show resolved Hide resolved
@amesgen amesgen mentioned this pull request Mar 16, 2023
@zzantares
Copy link
Contributor Author

Hey! apologies for the late response, I've applied the suggestions.

BTW if this is a breaking change and brings you guys much pain then feel free to skip this if it suits you; the only thing I really needed was to actually see what was the problem I was running into.

@mrkkrp
Copy link
Member

mrkkrp commented Mar 31, 2023

I believe #983 will warrant Ormolu 0.6.0.0, and so we can merge this without any concerns about backwards compatibility.

When ormolu fails to parse the Cabal file the user doesn't know what is the
actual error as only the cabal filepath is reported. This makes the necessary
changes to actually show what errors the cabal file parser encountered.

Before:
  Parsing this .cabal file failed:
    /path/to/file.cabal

After:
  Parsing this .cabal file failed:
    /path/to/file.cabal:0:0: Unsupported cabal-version 3.8. See haskell/cabal#4899.

Note: Removing the `Eq` instance on `OrmoluException` was the easy way to not
  having to implement an orphan `Eq` instance on `PError`. This does not have
  any impact elsewhere as we're actually not making use of the `Eq` instance
  on this exception type.
@mrkkrp mrkkrp force-pushed the report-cabal-parsing-errors branch from ef74b10 to d5ed151 Compare March 31, 2023 18:34
@mrkkrp mrkkrp merged commit 310f900 into tweag:master Mar 31, 2023
9 checks passed
@zzantares zzantares deleted the report-cabal-parsing-errors branch March 31, 2023 23:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants