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

Rethink 'CliEnvSettings' and 'defaultCliEnvSettings' #67

Closed
chshersh opened this issue Sep 1, 2022 · 3 comments · Fixed by #97
Closed

Rethink 'CliEnvSettings' and 'defaultCliEnvSettings' #67

chshersh opened this issue Sep 1, 2022 · 3 comments · Fixed by #97
Labels
enhancement New feature or request good first issue Good for newcomers hacktoberfest https://hacktoberfest.com/ question Further information is requested

Comments

@chshersh
Copy link
Owner

chshersh commented Sep 1, 2022

Currently, the CliEnvSettings data type is defined in the following way:

iris/src/Iris/Env.hs

Lines 53 to 71 in 0b9c50d

data CliEnvSettings (cmd :: Type) (appEnv :: Type) = CliEnvSettings
{ -- | @since 0.0.0.0
cliEnvSettingsCmdParser :: Opt.Parser cmd
-- | @since 0.0.0.0
, cliEnvSettingsAppEnv :: appEnv
-- | @since 0.0.0.0
, cliEnvSettingsHeaderDesc :: String
-- | @since 0.0.0.0
, cliEnvSettingsProgDesc :: String
-- | @since 0.0.0.0
, cliEnvSettingsVersionSettings :: Maybe VersionSettings
-- | @since 0.0.0.0
, cliEnvSettingsRequiredTools :: [Tool cmd]
}

And defaultCliEnvSettings:

iris/src/Iris/Env.hs

Lines 78 to 86 in 0b9c50d

defaultCliEnvSettings :: CliEnvSettings () ()
defaultCliEnvSettings = CliEnvSettings
{ cliEnvSettingsCmdParser = pure ()
, cliEnvSettingsAppEnv = ()
, cliEnvSettingsHeaderDesc = "Simple CLI program"
, cliEnvSettingsProgDesc = "CLI tool build with iris - a Haskell CLI framework"
, cliEnvSettingsVersionSettings = Nothing
, cliEnvSettingsRequiredTools = []
}

The idea of using () in the type signature was to rely on type-changing record updates. Unfortunately, the following code doesn't compile:

cmdP :: Parser Cmd
cmdP = ...

appSettings :: Iris.CliEnvSettings Cmd ()
appSettings = Iris.defaultCliEnvSettings
    { Iris.cliEnvSettingsCmdParser = cmdP
    }

It fails with the error:

... location ...
    • Couldn't match type ‘()’ with ‘Cmd’
      Expected: CliEnvSettings Cmd ()
        Actual: CliEnvSettings () ()
    • In the expression:
        Iris.defaultCliEnvSettings {cliEnvSettingsCmdParser = cmdP}
      In an equation for ‘appSettings’:
          appSettings
            = Iris.defaultCliEnvSettings {cliEnvSettingsCmdParser = cmdP}
   |
26 | appSettings = Iris.defaultCliEnvSettings
   |               ^^^^^^^^^^^^^^^^^^^^^^^^^^...

... location ...
    • Couldn't match type ‘Cmd’ with ‘()’
      Expected: Options.Applicative.Types.Parser ()
        Actual: Options.Applicative.Types.Parser Cmd
    • In the ‘cliEnvSettingsCmdParser’ field of a record
      In the expression:
        Iris.defaultCliEnvSettings {cliEnvSettingsCmdParser = cmdP}
      In an equation for ‘appSettings’:
          appSettings
            = Iris.defaultCliEnvSettings {cliEnvSettingsCmdParser = cmdP}
   |
27 |     { cliEnvSettingsCmdParser = cmdP
   |                                 ^^^^

The problem here because the cliEnvSettingsRequiredTools is also parametrised by cmd. So to change the type, you need to update both fields like this:

appSettings :: Iris.CliEnvSettings Cmd ()
appSettings = Iris.defaultCliEnvSettings
    { Iris.cliEnvSettingsCmdParser = cmdP
    , Iris.cliEnvSettingsRequiredTools = []
    }

Which is a shame. You need to set an extra field you don't care about all the time because you want to change the type of the CLI command.

It would be great to improve this interface. However, I don't have ideas at the moment. This requires some thinking 🤔

@chshersh chshersh added enhancement New feature or request question Further information is requested labels Sep 1, 2022
@chshersh chshersh changed the title Rething 'CliEnvSettings' and 'defaultCliEnvSettings' Rethink 'CliEnvSettings' and 'defaultCliEnvSettings' Sep 1, 2022
@chshersh
Copy link
Owner Author

chshersh commented Oct 1, 2022

Okay, I was thinking about the interface and here is the proposed solution:

  • Remove the cliEnvSettingsRequiredTools :: [Tool cmd] field from the CliEnvSettings options
  • Change the type of Tool to not have the cmd type parameter
  • Implement a separate function need :: Monad ... => [Tool] -> m ()

So, instead of specifying a global list of tools, users can call the need function in the required commands:

app :: App ()
app = Iris.asksCliEnv Iris.cliEnvCmd >>= \case
    Download url -> do
        need ["curl"]
        runDownload URL
    Evaluate hs -> do
        need ["ghc", "cabal"]
        runEvaluate hs

@german1608
Copy link
Contributor

Is anyone working on this? I can take a look otherwise 😄

@chshersh
Copy link
Owner Author

@german1608 I believe no one is looking (or at least haven't written about this explicitly). So feel free to work on this 🙂

@chshersh chshersh added this to the v0.1.0.0: Improved UX milestone Jan 8, 2023
chshersh added a commit that referenced this issue Jan 16, 2023
…iredTools` from `CliEnvSettings` (#97)

Resolves #67 

* Remove cmd parameter from Tool and remove cliEnvSettingsRequiredTools from CliEnvSettings
* Use MonadIO constraint instead of IO
* Fix missing field
* include name in customParserSettings
* Fix HLint unnecessary $ warning

Co-authored-by: Dmitrii Kovanikov <kovanikov@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers hacktoberfest https://hacktoberfest.com/ question Further information is requested
Projects
No open projects
Status: Done
2 participants