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

fieldWIthParser helper #31

Open
singpolyma opened this issue Dec 7, 2013 · 10 comments
Open

fieldWIthParser helper #31

singpolyma opened this issue Dec 7, 2013 · 10 comments

Comments

@singpolyma
Copy link
Contributor

If I have an existing parser, it would be nice to have an easy way to use that to parse database contents, like maybe:

fieldWithParser :: String -> (Text -> Either String a) -> RowParser a
fieldWithParser typ parse = fieldWith \f -> case fieldData f of
    (SQLText t) -> case parse t of
        Left e -> Errors [toException $ ConversionFailed "TEXT" typ e]
        Right v -> Ok v
    _ -> Errors [toException $ ConversionFailed "TEXT" typ]
@nurpax
Copy link
Owner

nurpax commented Dec 7, 2013

If I understand you right, I think a similar effect can be accomplished by wrapping your type with a newtype and adding a FromField instance for that. Here's a snippet from UserInstances.hs unit test:

newtype MyType = MyType String deriving (Eq, Show, Typeable)

instance FromField MyType where
  fromField f = cvt f . fieldData $ f where
    -- Prefix with "fromField " to really ensure we got here
    cvt _ (SQLText s) = Ok $ MyType ("fromField "++(T.unpack s))
    cvt f _           = returnError ConversionFailed f "expecting SQLText type"

I realize though that this does not cover everything. If the above is not what you'd like to do, can you post a comment with an end-to-end example of your suggestion?

@singpolyma
Copy link
Contributor Author

@nurpax That suggestion is basically the same as what I have to do write now, that is, I have to write the full adapter from the parser :: Text -> Either String a style functions that exist to a FieldParser by writing out all of the bits. A helper would allow me to just quickly write fieldWithParser "MyType" parser or similar.

@nurpax
Copy link
Owner

nurpax commented Jan 20, 2014

Sorry, this fell through the cracks. It'd be great to see a complete example of what you had to do without this feature (preferably one that compiles) and an equivalent example using the new entry point you're proposing.

@singpolyma
Copy link
Contributor Author

Current:

instance FromRow Deposit where
    fromRow = Deposit <$> field <*> field <*> fieldWith emailF <*> field <*> fieldWith rippleF <*> field <*> field
        where
        emailF f = case fieldData f of
            (SQLText t) -> case validate (encodeUtf8 t) of
                Left e -> Errors [toException $ ConversionFailed "TEXT" "EmailAddress" e]
                Right email -> Ok email
            _ -> Errors [toException $ ConversionFailed "TEXT" "EmailAddress" "need a text"]

        rippleF f = case fieldData f of
            (SQLText t) -> case readMay t of
                Nothing -> Errors [toException $ ConversionFailed "TEXT" "RippleAddress" "invalid"]
                Just ripple -> Ok ripple
            _ -> Errors [toException $ ConversionFailed "TEXT" "RippleAddress" "need a text"]

New:

instance FromRow Deposit where
    fromRow = Deposit <$> field <*> field <*> fieldWithParser "EmailAddress" (validate . encodeUtf8) <*> field <*> fieldWithParser "RippleAddress" (note "invalid" readMay) <*> field <*> field

@nurpax
Copy link
Owner

nurpax commented Jan 23, 2014

@singpolyma Sorry for not getting back to this earlier. I quite like the idea. The current FromField instance is kind of ugly as it uses the Ok type for the return value instead of Either.

Your suggestion

fieldWithParser :: String -> (Text -> Either String a) -> RowParser a

do you mind if we drop the first String parameter and require 'a' type to be an instance of Typeable? Typeable is already in heavy use in sqlite-simple and helps to construct better error messages automatically.

I originally thought perhaps we should generalize this so that non-string types could be used too, but that makes things a bit awkward. I'll post a WIP patch here and will try and if something better comes out. :)

@nurpax
Copy link
Owner

nurpax commented Jan 23, 2014

@lpsmith - any thoughts on generalizing or improving what's being proposed here? This might be useful for postgresql-simple too.

Example usage:

newtype CustomField = CustomField Double deriving (Eq, Show, Typeable)
data FooType = FooType String CustomField deriving (Eq, Show, Typeable)

customFromText :: T.Text -> Either String CustomField
customFromText = fmap (CustomField . fst) . T.rational

instance FromRow FooType where
  fromRow = FooType <$> field <*> fieldWithParser customFromText

If you hit an error in the fieldWithParser parser function, the error will look something like this:

ConversionFailed {errSQLType = "TEXT", errHaskellType = "CustomField", errMessage = "User defined parser in fieldParseText failed: input does not start with a digit"}

OTOH, if you hit a type error, you'll see something like this:

ConversionFailed {errSQLType = "INTEGER", errHaskellType = "CustomField", errMessage = "expecting SQLText for fieldParseText"}

nurpax added a commit that referenced this issue Jan 23, 2014
@singpolyma
Copy link
Contributor Author

@nurpax I thought you might want to switch to Typeable -- what about providing a fieldWithParser' or something that takes the string, and make the main fieldWithParser require Typeable and just act as a thin wrapper?

@lpsmith
Copy link

lpsmith commented Jan 25, 2014

Well, very early versions of postgresql-simple used Either, but I moved away from it for several reasons; if I recall correctly the lack of an Alternative instance being the biggest.

I guess I don't have a strong opinion on this proposal; it's purely a convenience function which I tend to be wary of adding, but not an immediate disqualification either.

@nurpax
Copy link
Owner

nurpax commented Feb 2, 2014

@singpolyma Right, I guess I agree that requiring users of sqlite-simple to add a Typeable instance to their types just to get error messages right seems a bit harsh. OTOH, I'm not a big fan of adding debug strings into the public API, there aren't any existing entry points which would have them.

How about we name this to just "textField"? The type should pretty well show its intended use. I'd like 'text' in the name since this is hardcoded for sqlite string fields.

I also noticed that postgresql-simple has a

queryWith :: ToRow q => RowParser r -> Connection -> Query -> q -> IO [r]

which sqlite-simple doesn't have. Filed #34 for adding these.

@nurpax
Copy link
Owner

nurpax commented Jun 14, 2015

queryWith_ was contributed recently in #43.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants