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

Write default implementation to getLogAction via logActionL #123

Closed
chshersh opened this issue May 9, 2019 · 7 comments
Closed

Write default implementation to getLogAction via logActionL #123

chshersh opened this issue May 9, 2019 · 7 comments
Labels
enhancement New feature or request Hacktoberfest https://hacktoberfest.digitalocean.com/ package:co-log-core For: co-log-core

Comments

@chshersh
Copy link
Contributor

chshersh commented May 9, 2019

So it will be enough to implement either logActionL or get + set.

@chshersh chshersh added the enhancement New feature or request label May 9, 2019
@chshersh chshersh added the package:co-log-core For: co-log-core label May 22, 2019
@vrom911 vrom911 added the Hacktoberfest https://hacktoberfest.digitalocean.com/ label Sep 29, 2019
@SanchayanMaity
Copy link
Contributor

SanchayanMaity commented Oct 4, 2019

@chshersh Is this something I can work on if my understanding is correct?

I see logActionL is a Lens. If I understand correctly, the get and set you mention refer to the getter and setter in the context of Lens? Depending on what operation I apply using logActionL, I can retrieve getLogAction and setLogAction from logActionL. Am I thinking correctly here? But, then the class seems to do just this and I was looking first at the instances.

Can you clarify a bit more. This is my first time looking at Has pattern.

@chshersh
Copy link
Contributor Author

chshersh commented Oct 4, 2019

@SanchayanMaity The HasLog typeclass is exactly about getting and setting LogAction in some environment data type. Lens is a smart object that can be used as a getter and a setter. If you're interested in understanding lens concept better, I can recommend the following blog post by @vrom911:

Since lens has getter and setter capabilities, it should be enough to define only logActionL lens in the HasLog typeclass. However, currently this is not the case, because getLogAction function doesn't have the default implementation, so you need to implement it anyway which seems redundant.

So, in simple words, this issue is about the following:

  1. Add default implementation of getLogAction.
  2. Change minimal complete definition accordingly:

https://github.com/kowainik/co-log/blob/434ee55f12d839e9c23909107d07c8866f0bfb7e/co-log-core/src/Colog/Core/Class.hs#L39-L40

@SanchayanMaity
Copy link
Contributor

SanchayanMaity commented Oct 5, 2019

@chshersh Does this look good?

Edit: Diff updated

diff --git a/co-log-core/src/Colog/Core/Class.hs b/co-log-core/src/Colog/Core/Class.hs
index d2f6c92..8aa2d8d 100644
--- a/co-log-core/src/Colog/Core/Class.hs
+++ b/co-log-core/src/Colog/Core/Class.hs
@@ -19,7 +19,7 @@ module Colog.Core.Class
        ) where
 
 import Colog.Core.Action (LogAction)
-
+import Data.Functor.Const (Const, getConst)
 
 -- to inline lens better
 {- HLINT ignore "Redundant lambda" -}
@@ -37,10 +37,12 @@ Every instance of the this typeclass should satisfy the following laws:
 4. __Set-Over:__ @'overLogAction' f env  'setLogAction' (f $ 'getLogAction' env) env@
 -}
 class HasLog env msg m where
-    {-# MINIMAL getLogAction, (setLogAction | overLogAction) #-}
+    {-# MINIMAL (getLogAction | logActionL) , (setLogAction | overLogAction) #-}
 
     -- | Extracts 'LogAction' from the environment.
     getLogAction :: env -> LogAction m msg
+    getLogAction = getConst . logActionL Const
+    {-# INLINE getLogAction #-}
 
     -- | Sets 'LogAction' to the given one inside the environment.
     setLogAction :: LogAction m msg -> env -> env

@chshersh
Copy link
Contributor Author

chshersh commented Oct 5, 2019

@SanchayanMaity A few notes:

  1. Imports must have explicit lists
  2. MINIMAL pragma doesn't look right. Now it says: logActionL and (setLogAction or overLogAction).

@SanchayanMaity
Copy link
Contributor

@chshersh Thank you for your patience & sorry for asking stupid questions.

@SanchayanMaity A few notes:

1. Imports must have explicit lists

If I do an import with explicit lists as below which is what I was trying to do

import Data.Functor.Const (Const, getConst)

I get an error, which I do not expect

co-log-core     >     • Data constructor not in scope:
co-log-core     >         Const
co-log-core     >           :: LogAction m0 msg0 -> Const (LogAction m msg) (LogAction m0 msg0)
co-log-core     >     • Perhaps you meant variable ‘const’ (imported from Prelude)
co-log-core     >       Perhaps you want to add ‘Const’ to the import list in the import of
co-log-core     >       ‘Data.Functor.Const’ (src/Colog/Core/Class.hs:22:1-43).
co-log-core     >    |
co-log-core     > 44 |     getLogAction = getConst . logActionL Const
co-log-core     >    |                                          ^^^^^
2. MINIMAL pragma doesn't look right. Now it says: `logActionL` and (`setLogAction` or `overLogAction`).

Either someone provides logActionL and we retrieve getLogAction from that or they provide getLogAction. Ditto for the other one. So below?

{-# MINIMAL (getLogAction | logActionL) , (setLogAction | overLogAction) #-}

@chshersh
Copy link
Contributor Author

chshersh commented Oct 5, 2019

@SanchayanMaity you need to import constructor of the Const data type. You can import everything you need for this issue by using the following import:

import Data.Const (Const (..))

So below?

Below doesn't look correct either. logActionL should be enough to implement the instance. So the pragma should have the shape: logActionL or smth else. Where smth else is what was before.

@vrom911
Copy link
Member

vrom911 commented Nov 8, 2019

Closed in #153

@vrom911 vrom911 closed this as completed Nov 8, 2019
@chshersh chshersh added this to the v0.4.0.0: Polishtown milestone Nov 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Hacktoberfest https://hacktoberfest.digitalocean.com/ package:co-log-core For: co-log-core
Projects
None yet
Development

No branches or pull requests

3 participants