Plumb rich failure types

[kostmo]

closes #1115

This PR makes use of the rich types defined in #982 and converts many functions of Has (Throw Text) type to ExceptT.

It also logs a warning instead of crashing with terminal failure if "save" info cannot be loaded, which will be important for #974 where the save file schema changes.

[xsebek]

Why do you curry error constructor? 🙁

As far as I see, you do not use the PathLoadFailure for anything else. The SystemFailure in question is already named AssetNotLoaded so what do you get by introducing this extra step?

[kostmo]

The SystemFailure type has an Asset member, and it was unclear in certain low-level functions which asset is relevant. However, in the higher-level functions, the context becomes clear, and then I am able to use withExceptT to supplement the error with the Asset argument.

[xsebek]

@kostmo I only see the getData[Dir|File]Safe functions. Why do this hard, when you can simply pass them the AssetData type?

- left (AssetNotLoaded $ Data AppAsset)  <$> getDataDirSafe "."
+ getDataDirSafe AppAsset "."

This also makes more sense, because no Asset other than AssetData is in the data directory.

[xsebek]

@kostmo thanks for working on this, once you are done I can rework logging to use these types. 👍

[xsebek]

@kostmo @byorgey why is this split from the Failure module? It depends on nothing else.

AFAIK we do not have any other modules that have data declaration and basic functions separately.

Look at Exception.hs - it has data Exn and formatExn in the same module.

[byorgey]

I was just commenting on a better place for the module to be, but I agree, it can/should probably just go in the Failure module itself.

[kostmo]

In this case I had intended to use quoting functions from Swarm.Util in the rendering code. And up until 75669d7, Swarm.Util depended on types in the Failure module, causing a circular dependency. So I did the work of splitting the modules, but later decided I didn't need the quoting functions after all.

However, after doing that splitting work, I'm not particularly inclined to merge Swarm.Failure and Swarm.Failure.Render back into a single module again---IMO, there is no downside to the types and the rendering functions being in separate modules, even if there is no technical requirement to be so. I've found it to be a common step in the evolution of a module: auxiliary functions that had been self-contained with the data definitions eventually needing to pull in an external dependency, requiring the data to move to a separate "types" module to break a circular reference. It may very well become necessary again for Failure rendering at some point.

It fact, for data definitions that pervade the codebase, it is convenient for changes to that data's rendering functions not to cause the whole project to recompile. The only use case I've observed that demands co-locating data and their "methods" (to borrow OOP parlance) in the same module is smart constructors. My stance has been that the partitioning of Haskell code into modules is largely arbitrary, but in general I prefer smaller modules, and even splitting them pre-emptively.

[xsebek]

@kostmo that is inconsistent with the code base style though.

Rendering widgets - sure.

But pure functions that are critical to usability of the type (ParseError and prettyParseError) can absolutely go into the same module and they do everywhere else in Swarm.

Which language puts pure functions on data in different module? In C++ and Python, I have always seen the functions that are basically “methods” in the same module.

Regarding dependencies, your reasoning is backwards. The Util module should simply not import anything else. It has to be on the bottom of dependency graph. We should try to move stuff from it (as you did) instead of importing modules in it.

[xsebek]

Finally from a practical standpoint this is premature and silly.

We have modules 2000+loc and argue here for splitting a 100loc module in a novel way, based on OOP best practise.

The future split could go another way though - split failures based on which part of game or UI produced it.
Whoever adds more code to failures in the future can decide which split fits better.

[xsebek]

As a side note, the smart constructor use case is a good point, but it does not apply here.

We will be adding failure types and their pretty printing at the same time, not tweaking the pretty printing function. This is not about "rendering a widget" - I am not going to change the colour of exceptions tomorrow and recompile the whole code base because Text does not have a colour Attr that I could change.

What I am trying to point out is that failure data type and format are tightly linked, think of it like C++ e.what() method instead of failureWidgetWithAttr.

[xsebek]

@kostmo I like how you split the modules 👍 Please just move them as I commented.

Some of this will be easier once Cabal enforces it on split libraries, but doing that is a lot of not-very-fun work, so I would appreciate it if you made it easier for me.

I can not wait to have it done and get to more enjoyable things like rewriting the logging. 😅

[byorgey]

Looking good to me!

[kostmo]

@xsebek, moved the new modules under Game. Good to go?

[xsebek]

@kostmo I think this is good enough to go. 👍

Please read my comment on PathLoadFailure though - it seems to me you are unnecessarily making the types harder to use.

I can not understand the urge to create separate "Render" modules, but I guess I can live with one extra folder and a file. I will just have to sort the folders and files alphabetically and not "folders first" so it is less annoying. 😒