proof-of-concept decode XLS from ByteString, typed cells #9
proof-of-concept decode XLS from ByteString, typed cells #9olafklinke wants to merge 14 commits intoharendra-kumar:masterfrom
Conversation
| , conduit >= 1.1 && < 1.4 | ||
| , filepath >= 1.0 && < 1.5 | ||
| , resourcet >= 0.3 && < 1.3 | ||
| , temporary |
There was a problem hiding this comment.
Can add bounds here. Also, bounds of transformers and resourcet can be bumped up.
| decodeXlsIO filePath | ||
|
|
||
| -- | Experimental: This function uses the @xls_open_buffer@ function of libxls. | ||
| decodeXlsByteString' :: ByteString -> IO (Either XLSError [[[Cell]]]) |
There was a problem hiding this comment.
- It would be better to have the same signature as decodeXlsByteString. The only difference between the two is exception vs either return. Both are IO, so can use exception.
- We can say in the docs, that it does not use a temporary file, instead uses ...
There was a problem hiding this comment.
So you suggest to make XLSError an instance of Exception and use throwIO instead of explicitly returning Either?
There was a problem hiding this comment.
It is somewhat odd that there are two exception types: XLSError from libxls and the XlsException defined in this package. Proposal: We add XLSError into the XlsException type so that only one exception type is thrown by this library.
There was a problem hiding this comment.
I created a branch in my fork for unifying the error types.
There was a problem hiding this comment.
I created a branch in my fork for unifying the error types.
|
@olafklinke just a few minor suggestions. I also added you to the repo, please check your email for the invite. Once we have resolved and merged this, can you please also upload it to hackage? Let me know your hackage user name, I will add you to maintainers. |
|
this is my hackage identity: olf |
harendra-kumar
left a comment
harendra-kumar
left a comment
There was a problem hiding this comment.
Looks good. Minor points. Plus a comment on the aux change in your fork.
| | TextCell String | ||
| | BoolCell Bool | ||
| | OtherCell o | ||
| deriving (Functor,Show,Eq) |
There was a problem hiding this comment.
Is there a way/possiblity for users to use this type? otherwise we should not expose this type and if that's the case then we can just use a Cell type instead of using it as a synonym to CellF (). Will keep things simple for users.
Bump version bounds to match upstream branch
3 participants
No description provided.