Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
proof-of-concept decode XLS from ByteString, typed cells #9
base: master
Are you sure you want to change the base?
proof-of-concept decode XLS from ByteString, typed cells #9
Changes from 7 commits
bc3f917
4925ca7
5e1a09a
e899008
4e7a2ba
f25086e
a76b586
49059e2
92828db
f56ee77
63a87ca
068856d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So you suggest to make
XLSError
an instance ofException
and usethrowIO
instead of explicitly returningEither
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is somewhat odd that there are two exception types:
XLSError
from libxls and theXlsException
defined in this package. Proposal: We addXLSError
into theXlsException
type so that only one exception type is thrown by this library.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I created a branch in my fork for unifying the error types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I created a branch in my fork for unifying the error types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 toCellF ()
. Will keep things simple for users.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can add bounds here. Also, bounds of transformers and resourcet can be bumped up.