Prep for 0.19: remove toString calls
#2
Conversation
stoeffel
force-pushed
the
019-tostring-nri
branch
3 times, most recently
from
363c0e1 to
1d76e28
Compare
| @@ -133,42 +148,46 @@ postPost body = | |||
| , withCredentials = | |||
| False | |||
| } | |||
| <<<<<<< HEAD | |||
src/Servant/Elm/Internal/Generate.hs
Outdated
| isElmStringType opts elmTypeExpr = | ||
| elmTypeExpr `elem` stringElmTypes opts | ||
|
|
||
|
|
||
| toStringSrcTypes :: T.Text -> ElmOptions -> ElmDatatype -> T.Text | ||
| toStringSrcTypes operator opts (ElmPrimitive (EMaybe argType)) = toStringSrcTypes operator opts argType | ||
| toStringSrcTypes _ _ (ElmPrimitive (EList (ElmPrimitive EChar))) = "identity" |
There was a problem hiding this comment.
It seems like this case would give a different type from the other cases, but I don't really understand how this function is supposed to work 😅. What does it do?
|
|
||
| {- | Determines whether we call `toString` on URL captures and query params of | ||
| this type in Elm. | ||
| -} | ||
| isElmStringType :: ElmOptions -> ElmDatatype -> Bool | ||
| isElmStringType _ (ElmPrimitive (EList (ElmPrimitive EChar))) = True |
There was a problem hiding this comment.
Is this saying that an elm List Char is an elm String? If so, how does that work?
There was a problem hiding this comment.
yes. don't know how it works, but it does 🙂
| @@ -40,9 +40,16 @@ data ElmOptions = ElmOptions | |||
| -- ^ Types that represent an empty Http response. | |||
| , stringElmTypes :: [ElmDatatype] | |||
| -- ^ Types that represent a String. | |||
| , intElmTypes :: [ElmDatatype] | |||
stoeffel
force-pushed
the
019-tostring-nri
branch
from
1d76e28 to
46ee69a
Compare
|
waiting with merge until I get the integration tests green. |
|
tests are broken on master, because of the last two commits. I've fixed all the tests in here, but I had to add the orphan instance back. I didn't want to deal with that in this PR. happy to pair on removing that again in the future. |
src/Servant/Elm/Internal/Orphans.hs
Outdated
|
|
||
| -- TODO: Generate Elm functions that can handle the response headers. PRs | ||
| -- welcome! | ||
| instance (ElmType a) => ElmType (Headers ls a) where |
There was a problem hiding this comment.
I don't think we want to add this back.
We moved the instance to elm-export: NoRedInk/elm-export@4b68744.
If we add this back, we've no clue which instance ghc will chose when it decides to choose one. We can hope and pray that the two instances line up, but if one of them changes, we're really messed up when ghc decides which instance to choose.
I think we should drop this instance again and do one of the following:
- Depend on our fork of
elm-export.
It doesn't look like our changes are going to be merged into upstream anytime soon, so no sense trying to keep parity with it. The tests will start compiling then. - Remove the test for
Headers.
If we can't support this data type, we shouldn't test against it.
No preference for either option, but I don't think re-adding this orphan is the way to go.
There was a problem hiding this comment.
Aside, this is a prime example of why working with these two dependencies has been frustrating for us. You start out wanting to make a seemingly innocuous change, and it ends up expanding into a whole web of problems.
There was a problem hiding this comment.
thanks for the explanation and proposed solutions. will address asap.
There was a problem hiding this comment.
@joneshf I removed the orphan and change the dependency to use our fork.
|
Looks great! |
Looks like tests on our master are broken.