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

Remove unused functions (or convince those functions are useful) #32

Closed
chshersh opened this issue Mar 5, 2017 · 7 comments
Closed

Comments

@chshersh
Copy link
Contributor

chshersh commented Mar 5, 2017

We have some functions in universum but we don't use them. Probably those functions are redundant. We can keep functions if we found some real uses cases or good usages (though it worth it to keep such functions in universum). So there're maybe 3 verdicts:

  1. Keep
  2. Remove
  3. Move to BARDAQ serokell-util

Here is full list of functions we don't use and personally I dont find useful:
orAlt orEmpty liftAA2 eitherA <<*>> guardM traceIO tryIO <<$>> liftM' liftM2' unsafeThrow leftToMaybe maybeToLeft applyN guarded guardedA

Here some functions we either use a little (but probably can be removed):
purer(1) NotImplemented(7, but actually don't use it) concatMapM(1) FatalError(2, #29) rightToMaybe(2) maybeToRight(1)

Some redundant things:

  1. Instances of Bifunctor for tuples length >= 3
@chshersh
Copy link
Contributor Author

chshersh commented Mar 5, 2017

Personally I want to keep <<$>> and guarded.

<<$>> is useful when you dealing with [[a]], Maybe [a], IO (Maybe a) and etc. Combination of two functors is very often. And writing fmap . fmap in that cases is really ugly.

guarded may be useful in some case when you want to work with some value and don't want to accidentally use not checked value:

safeSum :: Int -> Int -> Maybe Int
safeSum a b = do
    verifiedA <- guarded (>0) a
    verifiedB <- guarded (>0) b
    pure $ verifiedA + verifiedB

@gromakovsky
Copy link
Member

Regarding functions we don't use I don't mind if you remove all of them except <<$>> which I find useful too. Regarding guarded: I don't see anything bad if you leave it, semantics looks very intuitive.

@gromakovsky
Copy link
Member

NotImplemented is useful at early stage of development and it was used when started developing cardano-sl. Now it's not used and it's good, it basically means that all types are implemented. It's a stub for types which aren't designed yet.

concatMapM is intuitive but doesn't seem to be useful. It can be generalized, btw :)
https://hackage.haskell.org/package/UtilityTM-0.0.4/docs/Control-Monad-TM.html

FatalError and panic should be removed as I proposed earlier.

rightToMaybe is very useful I think. Btw, it's like hush, but with more concrete type.
maybeToRight seems less useful, but I don't mind having it in Universum.
Also if we remove leftToMaybe we can rightToMaybe to eitherToMaybe (dunno if it makes sense though).

Dunno about purer 🤷‍♂️

@chshersh
Copy link
Contributor Author

chshersh commented Mar 5, 2017

NotImplemented is useful at early stage of development and it was used when started developing cardano-sl. Now it's not used and it's good

That's the point. We don't want to keep things that are not intended to be used. Not every project uses NotImplemented at early development stage. And every such project can define it's own NotImplemented. So this data type can be removed from cardano-sl and from universum I think. But maybe it's ok to keep in universum, especially if you find it useful.

concatMapM is intuitive but doesn't seem to be useful. It can be generalized, btw :)

I like general version more :) So I insist on instead of removing we should introduce more general versions and remove UtilityTM-0.0.4 from dependencies.

Btw, it's like hush, but with more concrete type

Well, we have hush. But I also like functions with transparent semantics so maybe it's ok to leave it. At least these functions doesn't look crazy.

@gromakovsky
Copy link
Member

That's the point. We don't want to keep things that are not intended to be used. Not every project uses NotImplemented at early development stage. And every such project can define it's own NotImplemented.

As for me, NotImplemented is like notImplemented but for types. (After we removenotImplemented you can read it as undefined). We don't use notImplemented too (i. e. right now we don't have it in code), but we have it in universum.

Well, we have hush.

I am not sure if everybody is fine with having hush in universum. It's not a common function and its type is not obvious from type. If eventually we decide to remove hush then we should leave rightToMaybe at least.

@chshersh
Copy link
Contributor Author

chshersh commented Mar 5, 2017

We don't use notImplemented too (i. e. right now we don't have it in code), but we have it in universum.

Okay, that seems convincing. Maybe it's better to rename NotImplemented to Undefined for consistency.

I am not sure if everybody is fine with having hush

Also agree. We have only one usage of hush in current master of cardano-sl. I don't see much sense in such general version with such faceless name. Some similar useful functions can be also found here and taken from here: http://hackage.haskell.org/package/errors-2.1.3/docs/Control-Error-Util.html

Now I'm convinced that having rightToMaybe and company is ok.

@neongreen
Copy link
Contributor

neongreen commented Mar 5, 2017

Also agree. We have only one usage of hush in current master of cardano-sl. I don't see much sense in such general version with such faceless name.

+1

Regarding guarded: I don't see anything bad if you leave it, semantics looks very intuitive.

Agreed.

chshersh added a commit that referenced this issue Mar 8, 2017
chshersh added a commit that referenced this issue Mar 8, 2017
chshersh added a commit that referenced this issue Mar 9, 2017
chshersh added a commit that referenced this issue Mar 9, 2017
chshersh added a commit that referenced this issue Mar 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants