Parser (streamed): fix default value for cell type #154
Conversation
|
Thanks @NikitaRazmakhnin |
Sure, will do! :) |
According to specification if cell has no `t` attribute, it has a default value as `n` - number.
NikitaRazmakhnin
force-pushed
the
fix-stream-cell-type-parser
branch
from
d1e6e3e
to
1e60afe
Compare
|
@qrilka tests are done. I verified they fail with wrong value-type before fix and are green after the fix. |
| , IM.fromList [ (1, def & cellValue ?~ CellDouble 14.0 & cellStyle ?~ 1 ) ] | ||
| , IM.fromList [ (1, def & cellValue ?~ CellDouble 15.0) ] | ||
| ] | ||
| expected @==? (_ri_cell_row . _si_row <$> items) |
There was a problem hiding this comment.
What about checking the same for the non-streamed parser? It makes sense to ensure that they do the same thing.
There was a problem hiding this comment.
I wrote integration test around of that since I do not see an opportunity how
to test parseCell in more isolated manner, because parseCell are defined
deeply in functions of whole archive parsing.
I hope its fine for now to use it that way? I wrote same test for both parsers (Xeno and Cursored).
src/Codec/Xlsx/Parser/Stream.hs
Outdated
| @@ -688,7 +688,7 @@ parseStyle list = | |||
| parseType :: SheetValues -> Either TypeError ExcelValueType | |||
| parseType list = | |||
| case findName "t" list of | |||
| Nothing -> pure Untyped | |||
| Nothing -> Right TN -- according to spec by default if `t` is missing then default value is `n` - number | |||
There was a problem hiding this comment.
Maybe you could find a specific place in the spec talking about this? If no I could take a look a bit later
There was a problem hiding this comment.
Did I get you correctly that you propose to throw a reference to the spec here?
If so, I can do that no problems. Should I highlight it as well in comment for non-streamed parser?
There was a problem hiding this comment.
Yeah, section of the spec (or maybe reference to the schema?) would be great to prove the logic here or actually both places (or you could reference one from another)
There was a problem hiding this comment.
I am sorry, I placed wrong schema reference in the PR description (was reading specs in the first time
and was a bit confused by naming conventions). Now its clear to me what exactly to read :)
But I threw a correct ECMA references with paragraph numbers and quotes from xsd, because
could not find online format of that. Hope its fine!
Add spec refs from ECMA-376 around of `CellValue` definition and `CellValue` parsers.
|
@NikitaRazmakhnin please take a look into the failure with |
|
Thanks @NikitaRazmakhnin for the contribution! |
According to [ECMA-376] specification of XML schema of Excel default type for cell is
n- number.This is how it is implemented in regular parser, but streamed option was a bit incorrect here.
Spec declares such schema:
<xsd:attribute name="t" type="ST_CellType" use="optional" default="n"/>According to specification if cell has no
tattribute, it has a default value asn- number.But
Untypedcaused wrapping of numeric values into text instead.As far as I understand it should be
CellDoublein all cases wheretis omitted.