search

tweag / ormolu

A formatter for Haskell source code
https://ormolu-live.tweag.io
Other
944 stars 83 forks source link

Pretty print exceptions in tests #1031

Closed brandonchinn178 closed 1 year ago

brandonchinn178 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 :smile:

mrkkrp commented 1 year ago

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 commented 1 year ago

@mrkkrp done