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
Some comments #12
Comments
Hi Daniel, thanks for the review! Let’s go through the list, No FunctorI thought about this one, but decided it’s not a very obvious Functor instance so I left it away intentionally; I mention this in the documentation of Lists instead of
|
Thanks for your quick answer! 1 FunctorYes I missed this one, since I am already using it. I think for Haskeller it is quite natural to see that something is a functor. I also have noAnnotate in my library which is not a functor function. I think thats exactly the point reAnnotate vs unAnnotate, they are simplify different as such deserve different names :) If a library author provides typeclasses I also like to use them since they give me additional laws. While we are on it, you might also derive Generic and NFData. Are there other commonly used and useful typeclasses for this library. 2 FoldableIt is a matter of taste. Many people also don't like foldable if you consider the length discussions on reddit/cafe. Since my package uses foldable I have to try your package first to see what bites me more often - having foldable or having lists. But I am also using OverloadedLists and OverloadedStrings, so maybe I am not providing a good datapoint ;) concatWith looks good! 3 displayDecoratedI would really like to have this one. My argument above was meant in the way that they are not special at all. All my display functions basically rely on it. You can take a short look at Concerning the coloring - my worflow is as follows: I use displayDecorated to map the SimpleDoc to a list [Colored a]. See the type here. This list is then interpreted by a state machine in my coloring library. 4 Nearly empty packagesI understand that some of the packages are internal. Therefore you need these reexport modules. However at first sight it looked a bit complex. Maybe it could be a bit simpler. As it is now it introduces a too fine hierarchy in my opinion, Public, Half internal (the Internal.Types), Internal. Adding a short description to the module would certainly help. But what counts mostly is that the modules reflects the internal structure. For example your StackMachine is clearly separated from the rest, while the ShowS module doesn't reflect the structure. But in the end this is a matter of taste and of your personal cost function. For me empty modules have a high cost (too open the extra file etc) and full modules have a very high cost. You probably know gc.cpp ;) |
Functor
I agree that we like to see everything of kind Generic, NFDataGeneric is a good suggestion (and Typeable!), NFData I don’t think is a good idea, since it adds an additional dependency (on deepseq) and runs into the »NFData (a -> b)« problem. displayDecoratedMy intended workflow for this would be that you implement your own renderer using the utilities provided in the StackMachine/SimpleDocTree modules. Are the tutorials not covering your use case? I think you can just write a renderer from SimpleDoc(Tree) to [Color]. half internalI agree it’s not pretty, but I want to make clear that using Doc’s constructors is not something users should just go ahead and play around with. Exposing it at all is for a very specific use case I can point people to should they ask; for all others, I think »internal, don’t use or rely on« is good advice. |
|
Those definitions in your WL module do look really simple. I guess adding them to the StackMachine module won’t hurt, it will then contain the stack machine monad it contains now, and a couple of simpler API functions. |
cool thx! |
Done. Can we close this, or is there something else left to discuss? (If so, maybe a separate ticket is in order, otherwise the thread becomes too long.) |
Ok, thx for implementing the decorated function! I open some other issues. |
Hi David,
I just saw the release of your package on hackage. Looks pretty nice! I did something similar a short while ago based on ekmetts wl-pprint-extras since I needed something working with annotations: https://github.com/minad/wl-pprint-annotated and https://github.com/minad/wl-pprint-console. However your package looks better and more ambitious, so I might switch over :)
Some comments:
Daniel
The text was updated successfully, but these errors were encountered: