[#48] Add configurations, read from the file #59

vrom911 · 2018-05-23T10:53:31Z

Things to do

Handle [GhcVer] from the custom configurations
Add new instructions to the README explaining how configs should work

chshersh · 2018-05-23T14:29:09Z

src/Summoner.hs

@@ -1,6 +1,7 @@
 module Summoner
       ( module Summoner.Ansi
       , module Summoner.CLI
+       , module Summoner.Config
       , module Summoner.Default


Am I right that Default module will be removed?

Yes, it should be

chshersh · 2018-05-23T14:30:03Z

src/Summoner/CLI.hs

-     -- Generate the project.
-    generateProject projectName targets
+runWithOptions (InitOpts projectName cliConfig maybeFile) = do
+    file <- case maybeFile of


We really need universum for such things (so we can use whenNothing here) 😏

I'm agree with adding it after so many stupid imports..

chshersh · 2018-05-23T14:30:49Z

src/Summoner/CLI.hs

-    generateProject projectName targets
+runWithOptions (InitOpts projectName cliConfig maybeFile) = do
+    file <- case maybeFile of
+        Nothing -> getHomeDirectory >>= pure . (++ "/summoner.toml")


It's better to use (</>) operator instead of (++) to make it work on Windows as well.

Also, m >>= pure . f is f <$> m.

chshersh · 2018-05-23T14:35:23Z

src/Summoner/CLI.hs

+                Right conf -> pure $ defaultConfig <> conf
+        else pure defaultConfig
+     -- get the final config
+    let globalConfig = case finalise (config <> cliConfig) of


Personally, to me it looks slightly more cleaner when you write all configs in one place, like this:

let unionConfig = defaultConfig <> fileConfig <> cliConfig finalConfig <- ...

chshersh · 2018-05-23T14:35:46Z

src/Summoner/CLI.hs

+                    putStrLn "Using default config.."
+                    pure defaultConfig
+                Right conf -> pure $ defaultConfig <> conf
+        else pure defaultConfig


In order to make unionConfig you can just return pure mempty here.

chshersh · 2018-05-23T14:56:04Z

src/Summoner/Config.hs

+    fromDecision d = snd $ head $ filter ((== d) . fst) decisionMaybe
+
+    toDecision :: Maybe Bool -> Decision
+    toDecision m = fst $ head $ filter ((== m) . snd) decisionMaybe


lookup +swap here as well 👍

chshersh · 2018-05-23T15:00:35Z

src/Summoner/Config.hs

+    <*> pure cBench
+    <*> fin  "ghcVersions" cGhcVer
+  where
+    fin name = maybe (Left $ "Missing field: " <> name) Right . getLast


Actually, this is an interesting piece of code. Applicative instance for Either works in that way that it fails on first error and returns only single error to user. But it would be much more convenient to tell all absent fields at once. This actually already exist in Haskell world. There's data type called Validation. It's like Either (just different constructor names). But it has slightly more interesting applicative instance. It accumulates all errors. So we can copy-paste Validation to us and this instance and just use it here instead of Either to output multiple errors to user regarding absent fields!

chshersh · 2018-05-23T15:03:22Z

src/Summoner/Config.hs

+
+-- | Read configuration from the given file and return it in data type.
+loadFileConfig :: MonadIO m => FilePath -> m (Either Toml.EncodeException PartialConfig)
+loadFileConfig filePath = liftIO $ TIO.readFile filePath >>= pure . Toml.encode configT


I think you can just use throwIO here instead of returning Either. If there's error in configuration we don't want to skip this file, we want to throw exception. This also will simplify code a bit in main.

chshersh · 2018-05-23T15:04:44Z

src/Summoner/Project.hs

@@ -198,3 +133,23 @@ generateProject projectName Targets{..} = do
        "git" ["add", "."]
        "git" ["commit", "-m", "Create the project"]
        "git" ["push", "-u", "origin", "master"]
+
+    categoryText :: Text


Cool idea to move this under where.
You get the joke?) under where is like underwear 😂

chshersh · 2018-05-23T15:05:10Z

src/Summoner/ProjectData.hs

@@ -1,12 +1,22 @@
+{-# LANGUAGE DeriveAnyClass #-}
+{-# LANGUAGE DeriveGeneric  #-}
+{-# LANGUAGE InstanceSigs   #-}


I think DeriveGeneric and InstanceSigs should be put into default extensions safely.

chshersh · 2018-05-24T08:37:06Z

src/Summoner/Config.hs

+data LoadTomlException = LoadTomlException FilePath String
+
+instance Show LoadTomlException where
+    show (LoadTomlException filePath msg) = "Couldnt parse file " ++ filePath ++ ": " ++ show msg


Yeah, sorry for that... I will add Exception instance to EncodeException. But I think I can't write this code better. Because for now EncodeException contains file name only in case of parse error, not encoding error.

chshersh · 2018-05-24T08:58:45Z

README.md

+              `false` if you don't. If not specified it would be asked during each run of the `summoner`.
+* `lib` – `true` if you want to create `src` folder by default,
+          `false` if you don't. If not specified it would be asked during each run of the `summoner`.
+* `exe` – `true` if you want to create `app` folder by default,


I think it's good to add here that app folder will contain dummy Main.hs file and executable target will be added to .cabal file. Same for test flag.

chshersh · 2018-05-24T09:06:23Z

src/Summoner/CLI.hs

-    generateProject projectName targets
+runWithOptions (InitOpts projectName cliConfig maybeFile) = do
+    file <- case maybeFile of
+        Nothing -> (</> "summoner.toml") <$> getHomeDirectory


Maybe we can put "summoner.toml" into Default module. As well as IO function which returns full path to this file.

chshersh · 2018-05-24T09:19:59Z

src/Summoner/CLI.hs

+    fileConfig <-
+        if isFile
+        then loadFileConfig file
+        else pure mempty


After thinking a little bit more on this, I came to decision that logic here should be slightly complicated to increase UX.

If file exist, print information message (probably we can use Blue for information?) that this file configuration will be used.

If this is a default file ~/summoner.toml and it doesn't exist then print info message that no configuration file was specified (or will be used).

If this is not a default configuration file (i.e. specified by --file) then we should abort immediately because I think that if you specify file manually through CLI you expect this file to exist. But if you have typo in file name, it's better to see error that such file doesn't exist and fix CLI command instead of silently continue to work.

chshersh · 2018-05-25T07:33:31Z

src/Summoner/CLI.hs

+        then do
+            infoMessage $ "Configurations from " <> T.pack file <> " will be used."
+            loadFileConfig file
+        else  if isDefault


Extra space here..

chshersh · 2018-05-25T07:34:05Z

src/Summoner/CLI.hs

-             Success c    -> c
+    finalConfig <- case finalise unionConfig of
+             Failure msgs -> do
+                 for_ msgs $ \msg -> errorMessage msg


You can eta-reduce lambda here

chshersh · 2018-05-25T07:35:28Z

src/Summoner/CLI.hs

+            loadFileConfig file
+        else  if isDefault
+              then do
+                  warningMessage "Default config file is missing."


I think you can add path defaultTomlFile path to this message just in case. So stupid people won't complain in a way like It says to me that default config file is missing but I don't know where default config file should be.

chshersh · 2018-05-25T07:37:29Z

src/Summoner/Question.hs

-queryManyRepeatOnFail parser question = do
-    T.putStrLn question
+queryManyRepeatOnFail :: forall a . (Text -> Maybe a) -> IO [a]
+queryManyRepeatOnFail parser = do


You don't need do here and you can put promptLoop on the same line as queryManyRepeatOnFail

vrom911 added feature idea config TOML configurations labels May 23, 2018

vrom911 self-assigned this May 23, 2018

vrom911 requested a review from chshersh May 23, 2018 10:53

chshersh suggested changes May 23, 2018

View reviewed changes

chshersh reviewed May 24, 2018

View reviewed changes

chshersh reviewed May 25, 2018

View reviewed changes

vrom911 and others added 5 commits May 25, 2018 16:11

[#48] Add configurations, read from the file

3706091

Add Validation, refactor

1f4a0cc

Fix README, CHANGELOG, fix ghcVers in toml

7275f15

Change the logic of toml files, refactor coloring

f9444fc

Fixes

aebce9b

vrom911 force-pushed the vrom911/48-toml branch from f356caf to aebce9b Compare May 25, 2018 08:11

chshersh approved these changes May 25, 2018

View reviewed changes

chshersh merged commit a0e1507 into master May 25, 2018

chshersh deleted the vrom911/48-toml branch May 25, 2018 09:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#48] Add configurations, read from the file #59

[#48] Add configurations, read from the file #59

vrom911 commented May 23, 2018 •

edited by chshersh

Loading

chshersh May 23, 2018

vrom911 May 24, 2018

chshersh May 23, 2018

vrom911 May 24, 2018

chshersh May 23, 2018

chshersh May 23, 2018

chshersh May 23, 2018

chshersh May 23, 2018

chshersh May 23, 2018

chshersh May 23, 2018

chshersh May 23, 2018

vrom911 May 24, 2018

chshersh May 23, 2018

chshersh May 24, 2018

chshersh May 24, 2018

chshersh May 24, 2018

chshersh May 24, 2018

chshersh May 25, 2018

chshersh May 25, 2018

chshersh May 25, 2018

chshersh May 25, 2018

[#48] Add configurations, read from the file #59

[#48] Add configurations, read from the file #59

Conversation

vrom911 commented May 23, 2018 • edited by chshersh Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vrom911 commented May 23, 2018 •

edited by chshersh

Loading