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

Improve integration with lens #22

Closed
PierreR opened this Issue Feb 10, 2017 · 13 comments

Comments

Projects
None yet
3 participants
@PierreR
Contributor

PierreR commented Feb 10, 2017

AFAIK in its current form, if I want to re-use records with lenses, I have two options:

  • force all keys for records to start with _ in configuration files
  • append all keys with a prefix that will match the Haskell record

As a note using makeLensesWith abbreviatedFields won't work with records containing Vector field.

The first option is the easiest path. It is annoying in the sense that end-users will face _ in all record keys.

The second option not only forces the use of a namespace for each fields. For instance for such a record in a file 'box':

{ userName           = ""                                          
, userEmail            = ""                                         
, userStacks           = ["bos", "irisbox"]                        
, eclipsePlugins     = True                                        
...
}

It would be converted into:

{ boxUserName         = ""                                          
, boxUserEmail        = ""                                         
, boxUserStacks         = ["bos", "irisbox"]                        
, boxEclipsePlugins   = True                                        
....
}

But it also forces the name of the 'prefix' to match the name of the haskell record exactly. In this example, if I want to use BoxConfig instead of just Box in the haskell code I will be out of luck.

Is it possible to imagine that the Interpreter would be clever enough to optionally ignore 'underscore' from haskell record ?

data BoxConfig
  = BoxConfig
  { _userName        :: LText
  , _userEmail       :: LText
  , _userStacks      :: Vector LText
  , _eclipsePlugins  :: Bool
  } deriving (Generic, Show)

makeLenses ''BoxConfig
instance Interpret BoxConfig

main = do
  box_config <- input auto "./box"
  let test = box_config^.userEmail.strict

with

{ userName         = ""                                          
, userEmail          = ""                                         
, userStacks         = ["bos", "irisbox"]                        
, eclipsePlugins   = True  
}
@PierreR

This comment has been minimized.

Show comment
Hide comment
@PierreR

PierreR Feb 10, 2017

Contributor

Erratum: both makeLensesWith abbreviatedFields and makeLensesWith camelCaseFields will work on Vector when the FlexibleInstances extension is enabled.

So for the second option the haskell record doesn't need to match the prefix namespace.

Contributor

PierreR commented Feb 10, 2017

Erratum: both makeLensesWith abbreviatedFields and makeLensesWith camelCaseFields will work on Vector when the FlexibleInstances extension is enabled.

So for the second option the haskell record doesn't need to match the prefix namespace.

@PierreR

This comment has been minimized.

Show comment
Hide comment
@PierreR

PierreR Feb 10, 2017

Contributor

I took the option 2 road which is kind of ok.

If it is not too much trouble I still believe it would be nice if the Interpreter could optionally ignore 'underscore' from haskell record.

Contributor

PierreR commented Feb 10, 2017

I took the option 2 road which is kind of ok.

If it is not too much trouble I still believe it would be nice if the Interpreter could optionally ignore 'underscore' from haskell record.

@newhoggy

This comment has been minimized.

Show comment
Hide comment
@newhoggy

newhoggy Feb 21, 2017

Just bumped into this issue as well.

newhoggy commented Feb 21, 2017

Just bumped into this issue as well.

@Gabriel439

This comment has been minimized.

Show comment
Hide comment
@Gabriel439

Gabriel439 Feb 21, 2017

Collaborator

Yeah, I plan to fix this. I would like to support lens usage, too

Collaborator

Gabriel439 commented Feb 21, 2017

Yeah, I plan to fix this. I would like to support lens usage, too

Gabriel439 added a commit that referenced this issue Feb 22, 2017

Add support for customizing derived `auto` implementation. Fixes #22
This adds a new `deriveAuto` utility that you can customize with
`InterpretOptions` in order to transform the expected field or constructor
labels that Dhall expects

The most common use of this is to support data types that prefix field names
with underscores for lens support
@Gabriel439

This comment has been minimized.

Show comment
Hide comment
@Gabriel439

Gabriel439 Feb 22, 2017

Collaborator

I have a PR out with support for customizing the field and constructor labels that the derived generic implementation expects: #24

Let me know if that solves your issue. If it does then I'll merge

Collaborator

Gabriel439 commented Feb 22, 2017

I have a PR out with support for customizing the field and constructor labels that the derived generic implementation expects: #24

Let me know if that solves your issue. If it does then I'll merge

@PierreR

This comment has been minimized.

Show comment
Hide comment
@PierreR

PierreR Feb 22, 2017

Contributor

It works nicely with one record but I am experiencing a behaviour that I don't understand when I have two:

import           Dhall                        hiding (auto)

auto = deriveAuto
  (defaultInterpretOptions { fieldModifier = Data.Text.Lazy.dropWhile (== '_') })

data AdConfig
  = AdConfig
  { _loginId  :: LText
  , _password :: LText
  } deriving (Generic, Show)

makeLenses ''AdConfig

instance Interpret AdConfig

data BoxConfig
  = BoxConfig
  { _userName       :: LText
  , _userEmail      :: LText
  , _userStacks     :: Vector LText
  } deriving (Generic, Show)

makeLenses ''BoxConfig

instance Interpret BoxConfig

adConfig :: MonadIO m => m AdConfig
adConfig = liftIO $ Dhall.input auto "/vagrant/config/ad"

This code will fail to compile:

 Couldn't match type ‘BoxConfig’ with ‘AdConfig’
       Expected type: IO AdConfig
         Actual type: IO BoxConfig
     • In the second argument of ‘($)’, namely
         ‘(Dhall.input auto "/

As soon as I stop hiding auto and comment the overloaded definition it compiles again.

Contributor

PierreR commented Feb 22, 2017

It works nicely with one record but I am experiencing a behaviour that I don't understand when I have two:

import           Dhall                        hiding (auto)

auto = deriveAuto
  (defaultInterpretOptions { fieldModifier = Data.Text.Lazy.dropWhile (== '_') })

data AdConfig
  = AdConfig
  { _loginId  :: LText
  , _password :: LText
  } deriving (Generic, Show)

makeLenses ''AdConfig

instance Interpret AdConfig

data BoxConfig
  = BoxConfig
  { _userName       :: LText
  , _userEmail      :: LText
  , _userStacks     :: Vector LText
  } deriving (Generic, Show)

makeLenses ''BoxConfig

instance Interpret BoxConfig

adConfig :: MonadIO m => m AdConfig
adConfig = liftIO $ Dhall.input auto "/vagrant/config/ad"

This code will fail to compile:

 Couldn't match type ‘BoxConfig’ with ‘AdConfig’
       Expected type: IO AdConfig
         Actual type: IO BoxConfig
     • In the second argument of ‘($)’, namely
         ‘(Dhall.input auto "/

As soon as I stop hiding auto and comment the overloaded definition it compiles again.

@PierreR

This comment has been minimized.

Show comment
Hide comment
@PierreR

PierreR Feb 22, 2017

Contributor

I have realized that if I inline deriveAuto ... at each place it will work but as soon as I want to re-use it I will face the compilation problem.

Contributor

PierreR commented Feb 22, 2017

I have realized that if I inline deriveAuto ... at each place it will work but as soon as I want to re-use it I will face the compilation problem.

@PierreR

This comment has been minimized.

Show comment
Hide comment
@PierreR

PierreR Feb 22, 2017

Contributor

This probably means that I am supposed to write :

interpretDhallOptions = defaultInterpretOptions { fieldModifier = Data.Text.Lazy.dropWhile (== '_') }

adConfig :: MonadIO m => m AdConfig
adConfig = liftIO $ (Dhall.input (deriveAuto interpretDhallOptions) "/vagrant/config/ad")

Why not exposing a lensInterpretOptions to make the usage clearer ?

Contributor

PierreR commented Feb 22, 2017

This probably means that I am supposed to write :

interpretDhallOptions = defaultInterpretOptions { fieldModifier = Data.Text.Lazy.dropWhile (== '_') }

adConfig :: MonadIO m => m AdConfig
adConfig = liftIO $ (Dhall.input (deriveAuto interpretDhallOptions) "/vagrant/config/ad")

Why not exposing a lensInterpretOptions to make the usage clearer ?

@Gabriel439

This comment has been minimized.

Show comment
Hide comment
@Gabriel439

Gabriel439 Feb 22, 2017

Collaborator

@PierreR You are being bit by the monomorphism restriction. You can either enable {-# LANGUAGE NoMonomorphismRestriction #-} or you can give auto an explicit type signature and it will do the correct thing:

auto :: (GenericInterpret (Rep a), Generic a) => Type a
auto = deriveAuto
  (defaultInterpretOptions { fieldModifier = Data.Text.Lazy.dropWhile (== '_') })
Collaborator

Gabriel439 commented Feb 22, 2017

@PierreR You are being bit by the monomorphism restriction. You can either enable {-# LANGUAGE NoMonomorphismRestriction #-} or you can give auto an explicit type signature and it will do the correct thing:

auto :: (GenericInterpret (Rep a), Generic a) => Type a
auto = deriveAuto
  (defaultInterpretOptions { fieldModifier = Data.Text.Lazy.dropWhile (== '_') })
@PierreR

This comment has been minimized.

Show comment
Hide comment
@PierreR

PierreR Feb 22, 2017

Contributor

@Gabriel439 Thanks ! This monomorphism restriction always comes as a surprise ;-)

Contributor

PierreR commented Feb 22, 2017

@Gabriel439 Thanks ! This monomorphism restriction always comes as a surprise ;-)

@PierreR

This comment has been minimized.

Show comment
Hide comment
@PierreR

PierreR Feb 22, 2017

Contributor

As an extra note, GenericInterpret is not in scope so you are leaving me only one choice ...

Contributor

PierreR commented Feb 22, 2017

As an extra note, GenericInterpret is not in scope so you are leaving me only one choice ...

@PierreR

This comment has been minimized.

Show comment
Hide comment
@PierreR

PierreR Feb 23, 2017

Contributor

A part from these minor points, please merge, this is such a nice improvement. Thanks for the PR.

Contributor

PierreR commented Feb 23, 2017

A part from these minor points, please merge, this is such a nice improvement. Thanks for the PR.

@Gabriel439

This comment has been minimized.

Show comment
Hide comment
@Gabriel439

Gabriel439 Feb 23, 2017

Collaborator

The missing GenericInterpret is my mistake. I will include that in the export list and then merge

Collaborator

Gabriel439 commented Feb 23, 2017

The missing GenericInterpret is my mistake. I will include that in the export list and then merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment