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

Added hPutStr and hPutStrLn to the Print module #197

Merged
merged 4 commits into from
Oct 24, 2018

Conversation

JustusAdam
Copy link
Contributor

@JustusAdam JustusAdam commented Oct 3, 2018

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 gromakovsky added type:feature Something new is added. type:breaking Breaking change (removal, renaming, semantic change, etc.) and removed type:feature Something new is added. labels Oct 4, 2018
@gromakovsky
Copy link
Member

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:

  1. 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.
  2. putStr and putStrLn are not specialized to IO, I think it would be good to specialize them.

@JustusAdam
Copy link
Contributor Author

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:

  1. 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.
  2. 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
Copy link
Member

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
Copy link
Contributor

volhovm commented Oct 14, 2018

@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.

Copy link
Member

@gromakovsky gromakovsky left a comment

Choose a reason for hiding this comment

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

Please add hPrint and I'll approve.

JustusAdam added a commit to JustusAdam/universum that referenced this pull request Oct 15, 2018
JustusAdam added a commit to JustusAdam/universum that referenced this pull request Oct 15, 2018
As per the suggestion of @gromakovsky
Also added some docs
Also added the requested SPECIALIZE pragmas
JustusAdam added a commit to JustusAdam/universum that referenced this pull request Oct 15, 2018
Used documentation similar to `print`
Added SPECIALIZE pragma
Copy link
Member

@gromakovsky gromakovsky left a comment

Choose a reason for hiding this comment

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

Oh, forgot one last thing: changelog update.

Copy link
Contributor

@volhovm volhovm left a comment

Choose a reason for hiding this comment

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

👌

JustusAdam added a commit to JustusAdam/universum that referenced this pull request Oct 24, 2018
As per the suggestion of @gromakovsky
Also added some docs
Also added the requested SPECIALIZE pragmas
Used documentation similar to `print`
Added SPECIALIZE pragma
Copy link

@Haskell-mouse Haskell-mouse left a comment

Choose a reason for hiding this comment

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

Looks good. I thought about multiparameter type class "Print", but it would be unnesessary complication....

@gromakovsky gromakovsky merged commit 2664f23 into serokell:master Oct 24, 2018
JustusAdam added a commit to JustusAdam/universum that referenced this pull request Oct 28, 2018
JustusAdam added a commit to JustusAdam/universum that referenced this pull request Oct 28, 2018
As per the suggestion of @gromakovsky
Also added some docs
Also added the requested SPECIALIZE pragmas
JustusAdam added a commit to JustusAdam/universum that referenced this pull request Oct 28, 2018
Used documentation similar to `print`
Added SPECIALIZE pragma
JustusAdam added a commit to JustusAdam/universum that referenced this pull request Oct 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:breaking Breaking change (removal, renaming, semantic change, etc.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants