[WIP] Scaffolding of various features #1453
Conversation
Signed-off-by: Mihovil Ilakovac <[email protected]>
Signed-off-by: Mihovil Ilakovac <[email protected]>
There was a problem hiding this comment.
@infomiho this looks quite good to me!
Most of the comments are on extracting the logic.
I like the general structure and think it makes sense.
One thing that is not great is that we are repeating here quite a few stuff from Wasp -> this will be something we need to maintain separately, as compiler doesn't catch most of the stuff. It could catch quite more, but we are not there yet / it would require more effort. So it is a thing to keep updated, same like docs. But ok.
The fact that we are not updating main.wasp file -> yop that sucks a bit, but I think this is already valuable as the first version and we can leave that for later, since we said that is not as simple and would require quite more effort. I think this is already useful as it is.
| @@ -20,6 +20,7 @@ data Call | |||
| | WaspLS | |||
| | Deploy Arguments -- deploy cmd passthrough args | |||
| | Test Arguments -- "client" | "server", then test cmd passthrough args | |||
| | UseRecipe Arguments -- pass through args | |||
There was a problem hiding this comment.
What are the pass through args here, why would I want to pass them through?
| showOption = show | ||
| showOptionDescription = Just . recipeDescription | ||
|
|
||
| useRecipe :: [String] -> Command () |
There was a problem hiding this comment.
If there are no args, I would just remove _args argument for now.
| import qualified Wasp.Cli.Interactive as Interactive | ||
| import qualified Wasp.Message as Msg | ||
|
|
||
| data AuthMethod = Email | UsernameAndPassword | Google | Github |
There was a problem hiding this comment.
Are we replicating knowledge from AppSpec here? This is duplication, why not directly use the ADT from AppSpec?
If reason is that you wanted to be able to define Show and Interactive.Option instances for it, then you are better of doing newtype AuthMethodRecipe = AuthMethodRecipe AppSpec.Auth.AuthMethod -> wrapping the AppSpec ADT into a newtype that you will use here and write instances for it.
| @@ -0,0 +1,52 @@ | |||
| {-# LANGUAGE InstanceSigs #-} | |||
There was a problem hiding this comment.
Aha, you like having signature in the instance? I understand that, I also like them but I never pull the trigger on importing this extension hm, not sure why. Sounds good though.
| let authTargetDir = waspProjectDir </> [reldir|src/client/auth|] | ||
| return authTargetDir |
There was a problem hiding this comment.
| let authTargetDir = waspProjectDir </> [reldir|src/client/auth|] | |
| return authTargetDir | |
| return $ waspProjectDir </> [reldir|src/client/auth|] |
| import qualified Wasp.Message as Msg | ||
| import qualified Wasp.Util.Terminal as Term | ||
|
|
||
| data EmailProvider = SMTP | SendGrid | Mailgun |
There was a problem hiding this comment.
Also, consider using what we already have in AppSpec instead of repeating it here.
| useProvider SMTP = useEmailProvider "SMTP" ["SMTP_HOST", "SMTP_PORT", "SMTP_USERNAME", "SMTP_PASSWORD"] | ||
| useProvider SendGrid = useEmailProvider "SendGrid" ["SENDGRID_API_KEY"] | ||
| useProvider Mailgun = useEmailProvider "Mailgun" ["MAILGUN_API_KEY", "MAILGUN_DOMAIN"] |
There was a problem hiding this comment.
Would be great if this was not the only place where we have these env values encoded, but instead it was somewhere else and then we just used it here. Since it sounds like a pretty central concept.
It is the question of knowledge -> where should this knowledge be centralized?
| joinForSentence :: [String] -> String | ||
| joinForSentence [] = "" | ||
| joinForSentence [x] = x | ||
| joinForSentence [x, y] = x <> " and " <> y | ||
| joinForSentence (x : xs) = x <> ", " <> joinForSentence xs |
| cliSendMessageC $ | ||
| Msg.Info $ | ||
| unlines | ||
| [ "Make sure the following to your Main.css file:", |
| @@ -96,6 +104,12 @@ askToChoose question options = do | |||
| Just description -> "\n" <> replicate indentLength ' ' <> description | |||
| Nothing -> "" | |||
|
|
|||
| renderDefaultOption :: o -> String | |||
There was a problem hiding this comment.
A bit wild that it renders "" if there is no default option, as I would expect it to render default option.
I think it would be better if it returned Maybe String, so it returns Nothing if there is no default option?
Working on:
Biggest issues atm: