Implement more Effects #8
Conversation
|
|
platform/main-manual-glue.roc
Outdated
There was a problem hiding this comment.
OK I think I finally see what's happening, the name was misleading me.
Maybe it would be good to rename this to "extra" glue rather than "manual" glue.
There was a problem hiding this comment.
Actually even better would be to make it more descriptive. All the listed types seem to be "internal" to the platform. So maybe we could have "app" glue and "platform" glue?
There was a problem hiding this comment.
I like extra glue, I'll update it.
There was a problem hiding this comment.
This is fantastic to see! Great work!
A few suggestions below but no problems.
FYI, it's possible to tell GitHub which files are generated so that they are collapsed in PRs.
https://docs.github.com/en/repositories/working-with-files/managing-files/customizing-how-changed-files-appear-on-github
platform/main-glue.roc
Outdated
There was a problem hiding this comment.
If you need a separate file for glue anyway (because of the Task problem) then you could just combine all the app and host types into a single glue.roc.
But I suppose there's not much real benefit. And you might want to keep them separate for when the bug is fixed.
There was a problem hiding this comment.
Yeah, keeping seperate so when we fix the bugs we can just delete glue_manual crate and the glue mains.
| Effect.commandOutput (Box.box cmd) | ||
| |> Effect.map Ok | ||
| |> Effect.map (\out -> Ok out) |
There was a problem hiding this comment.
Oh wow, why was this needed? Compiler bug?
There was a problem hiding this comment.
Yes, this is a compiler bug. I am pretty sure this is known as it came up somewhere else earlier. @ayazhafiz or @bhansconnect do you know if this is new? should I make an issue for it?
| ## when output.status is | ||
| ## Ok {} -> jsonResponse output.stdout | ||
| ## Err _ -> byteResponse 500 output.stderr | ||
| ## ``` |
There was a problem hiding this comment.
Always nice to see doc comments being added!
examples/http.roc
Outdated
|
|
||
| readUrlEnv : Task Str AppError | ||
| readUrlEnv = | ||
| maybeDbPath <- Env.var "TARGET_URL" |> Task.attempt |
There was a problem hiding this comment.
This variable name is not an obvious choice. Does it have something to do with databases? Or is it copy-pasted from elsewhere?
There was a problem hiding this comment.
Not really, I was just looking for an excuse to read an Env.var so I could test that effect. I'll polish it some more in a follow up.
examples/http.roc
Outdated
| # Log the date, time, method, and url to stdout | ||
| {} <- logRequest req |> Task.await | ||
|
|
||
| # Read TARGET_URL environment variable |
There was a problem hiding this comment.
The fact that you had to make this comment suggests that the function call itself could be made a bit more self-explanatory.
Maybe it would be better to pass the env var name as a string so that you don't need the comment. Or rename the function.
There was a problem hiding this comment.
Thank you, great idea I'll add in a follow up PR
examples/http.roc
Outdated
| Ok str -> respond 200 str | ||
| Err _ -> respond 500 "Error 500 Internal Server Error\n" | ||
| Ok content -> Task.ok content | ||
| Err err -> Task.err (HttpError err) |
There was a problem hiding this comment.
I see a lot of these wrapper functions to transform things from Result to Task.
It might be worth considering absorbing those into the platform itself.
Not for this PR though, it's doing enough! Just an idea for later.
There was a problem hiding this comment.
In fact they are transforming from Task to Result and back to Task.
I don't understand why that's needed.
Is it to change the error type? Would Task.mapError achieve the same thing?
There was a problem hiding this comment.
I don't have a particular reason other than I was in a hurry and this was the first thing that came to mind when refactoring out the errors to have a common type and a single handler.
I would like to look at this closer in a following PR.
This PR
platform/glue-gen.shscript to capture the key steps