search

TravisWhitaker / rdf

Representation and Incremental Processing of RDF Data
MIT License
8 stars 6 forks source link

expose IRI parsing function #2

Closed odanoburu closed 5 years ago

odanoburu commented 5 years ago

the library exposes a few attoparsec parsers in Data.RDF.Parser.Common, among them the IRI parser; however (as far as I know) I can only actually parse something safely by importing attoparsec in user code. since user code might not (directly) depend on attoparsec, I think it would be nice to offer such parsing a function in the library. (plus if the underlying parsing library changes with no change of interface, user code doesn't break.)

thanks for the library!

TravisWhitaker commented 5 years ago

How do you think such a function should work? Which string types should it work with ((strict | lazy) (bytestring | text))? Should we use parse to return a Result IRI or parseOnly to return an Either String IRI?

There weren't clear answers to any of these questions, and I never needed to parse bare IRIs, so I just exported the Parsers. What's your use case?

odanoburu commented 5 years ago

There weren't clear answers to any of these questions

I see your point!

I want the user to be able to determine the baseIRI of the rdf being generated, and this will come from IO.

to answer your questions, instead of providing all possible interface combinations, I'd offer the most flexible ones (I don't care about converting from string or bytestring to strict text because this is going to be a one-time thing). I'd also go with Either since the other option would entail using attoparsec (if the user wants more control than that it might be time to make attoparsec a direct dependency)

does that make sense? what do you think?

TravisWhitaker commented 5 years ago

Rather than exporting an API that encourages the caller to unnecessarily convert between string types (unless we export all options, this will always be true for some callers), I think it's better if the caller imports the appropriate Attoparsec module for the type they're working with and simply does something like:

parseOnly (parseIRI <* endOfInput) :: Text -> Either String IRI

Dependencies will already be linking against attoparsec in order to link against this library anyway, and I don't think the parseOnly function is going anywhere anytime soon.

TravisWhitaker commented 5 years ago

There is also an IsString instance for IRI.

odanoburu commented 5 years ago

alright!