Added hPutStr and hPutStrLn to the Print module

JustusAdam commented 5 years ago

Problem

I was rather surprised to find that Universum does not export a version of hPutStrLn and hPutStr etc.

Solution

This PR adds both hPutStr as well as hPutStrLn to Universum.Print. putStr as well as putStrLn are still available and exported from Universum.Print.

Implementation considerations

I decided, in the spirit of a smaller surface area and code of Universum.Print, to completely switch the Print class methods.

My logic was that this is a class people rarely implement themselves. (It may perhaps even be prudent not to export hPutStrLn and hPutStr as methods but as normal functions instead).

I am open for comments and improvement suggestions though.

Alternatives

Additional `HPrint` typeclass

class HPrint a where
    hPutStrLn :: MonadIO m => Handle -> a -> m ()
    hPutStr :: MonadIO m => Handle -> a -> m ()

class Print a where
    putStr :: MonadIO m => a -> m ()
    default putStr :: (MonadIO m, HPrint a) => a -> m ()
    putStr = hPutStr stdout
    putStrLn :: MonadIO m => a -> m ()
    default putStrLn :: (MonadIO m, HPrint a) => a -> m ()
    putStrLn = hPutStrLn stdout

An extra type class that encodes printing to handles. This avoids the problem of possibly breaking backwards compatibility by changing the signature and names of methods in Print but means more code and a larger surface area for the module.

The methods for Print may also have default signatures that utilize HPrint

gromakovsky commented 5 years ago

First of all, thank you for contribution. I think that adding hPutStrLn and hPutStr to Universum makes sense, these functions have pretty clear semantics, they are standard and their names are unlikely to conflict with something different. I like how it's implemented, but it's indeed a breaking change, so we need more opinions on that. I think this change is ok to make in the next breaking release (1.5.0), because I think defining instance of Print is a quite rare case.

My logic was that this is a class people rarely implement themselves. (It may perhaps even be prudent not to export hPutStrLn and hPutStr as methods but as normal functions instead).

I've been thinking about that and I realized that it might be a good idea to have a module called e. g. Universum.Print.Internal which would export Print type class, while Universum.Print would only export functions, but not the type class itself. And not re-export Universum.Print.Internal, but expose it from the library. This way it will still be possible to define an instance of Print, but it will require importing Internal module in which we can make breaking changes more often.

Apart from that, I think there are two more things which can be improved in the Print module:

We have to use liftIO in each instance declaration, but instead we can have IO in type class definition and life these functions outside the type class. Not a big difference, but I think it will remove some duplication.
putStr and putStrLn are not specialized to IO, I think it would be good to specialize them.

JustusAdam commented 5 years ago

I pushed a version that implements the Print-as-internal-class suggestion of yours. While I was at it I also added some documentation. What do you think?

A few things that came to me while I was writing it:

Should we also have something like putErr and putErrLn that print a string to stderr? At least as far as I am concerned that's a pretty common operation.
Should there be an hPrint?

Also I chose to name the methods of Print the same as its overloaded versions. Again, I don't suspect many people will ever implement its methods and thus encounter the name clash so I think we'd be safe there.

gromakovsky commented 5 years ago

Should we also have something like putErr and putErrLn that print a string to stderr?

I've been thinking about it and currently I'd rather not add these functions because they are not standard (I found functions with these names only in some packages which I've never seen before). It's not bad per se, but earlier we realized that we have too many non-standard functions which make it harder for new people to get used to Universum, so we decided to add non-standard functions only if there several people which come and say "hey, this is so common, please add it". Currently you are the only such person. So I think it's better to open a new issue about putErr and putErrLn and let's see how it goes.

Should there be an hPrint?

I think it's fine to add hPrint, it's present in base and its name is descriptive enough.

volhovm commented 5 years ago

@JustusAdam thank you for the PR. Regarding your last questions:

Let's not introduce stderr printing. I'm not sure why do you consider it to be common, but, frankly, I haven't ever encountered anything related, at least in the production code. Usually, if you start caring about where do you print your stuff to, you pick a logging library. These printing functions are used in very small projects, for debugging, and app bootstrapping (when the logging is not yet initialised).
hPrint is fine for the reasons @gromakovsky has presented.

serokell / universum