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

Pretty print exceptions in tests #1031

Merged
merged 1 commit into from
May 23, 2023

Conversation

brandonchinn178
Copy link
Collaborator

No description provided.

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.

Nice, thanks! A variant of this PR would be to adapt the displayException implementation of OrmoluException, in which case the implementation of withNiceExceptions would stay as it is:

diff --git a/src/Ormolu/Exception.hs b/src/Ormolu/Exception.hs
index c7db297..38fb022 100644
--- a/src/Ormolu/Exception.hs
+++ b/src/Ormolu/Exception.hs
@@ -47,7 +47,8 @@ data OrmoluException
     OrmoluFixityOverridesParseError (ParseErrorBundle Text Void)
   deriving (Show)
 
-instance Exception OrmoluException
+instance Exception OrmoluException where
+  displayException = T.unpack . runTermPure . printOrmoluException
 
 -- | Print an 'OrmoluException'.
 printOrmoluException ::

@brandonchinn178
Copy link
Collaborator Author

I didn't want to do that, as changing an instance implementation would be a breaking change. And given that that instance hasn't changed in 3 years, I was hesitant to do so. But if you'd like, I can change

@amesgen
Copy link
Member

amesgen commented May 20, 2023

changing an instance implementation would be a breaking change

Maybe I am missing sth, but why would changing the returned string from displayException constitute a breaking change?

@brandonchinn178
Copy link
Collaborator Author

brandonchinn178 commented May 20, 2023

Would it not fall under "Did the behavior of any exported functions change?" in the flowchart in https://pvp.haskell.org/#decision-tree? Since instances are global, I would imagine instance functions are treated the same as exported functions

@amesgen
Copy link
Member

amesgen commented May 20, 2023

Thanks, interesting, was not aware of that, seems a bit strong to require a major version bump for any change in behavior of any exported function; in the most strict interpretation, that would mean that you can only fix (even minor) bugs in your functions by releasing a new major version. A probably more charitable interpretation would be that one should release a new major version when changing the behavior of an exported function in a way that users are likely to rely on significantly, which I don't think would be the case here.

The prose above also does not mention this case as a trigger for breaking changes:

Breaking change. If any entity was removed, or the types of any entities or the definitions of datatypes or classes were changed, or orphan instances were added or any instances were removed, then the new A.B MUST be greater than the previous A.B. Note that modifying imports or depending on a newer version of another package may cause extra orphan instances to be exported and thus force a major version change.

In any case, let's just wait and see what e.g. @mrkkrp thinks 😄

@mrkkrp
Copy link
Member

mrkkrp commented May 22, 2023

I think we should probably go with adjusting the implementation of displayException. Haddocks say:

Render this exception value in a human-friendly manner.

Which is not what we currently do, so let's correct it.

I think it is acceptable to change the implementation, I imagine that not many people rely on this.

@brandonchinn178
Copy link
Collaborator Author

@mrkkrp done

@mrkkrp mrkkrp merged commit 01cb1e8 into tweag:master May 23, 2023
9 checks passed
@brandonchinn178 brandonchinn178 deleted the test-exceptions branch May 23, 2023 07:45
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