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

Replace Double in CellValue by Scientific #177

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
4 changes: 2 additions & 2 deletions src/Codec/Xlsx/Parser.hs
Original file line number Diff line number Diff line change
Expand Up @@ -336,7 +336,7 @@ extractSheetFast ar sst contentTypes caches wf = do
Nothing -> throwError "bad shared string index"
"inlineStr" -> mapM (fmap xlsxTextToCellValue . fromXenoNode) isNode
"str" -> fmap CellText <$> vConverted
"n" -> fmap CellDouble <$> vConverted
"n" -> fmap CellDecimal <$> vConverted
"b" -> fmap CellBool <$> vConverted
"e" -> fmap CellError <$> vConverted
unexpected ->
Expand Down Expand Up @@ -514,7 +514,7 @@ extractCellValue sst t cur
| t == "inlineStr" =
cur $/ element (n_ "is") >=> fmap xlsxTextToCellValue . fromCursor
| t == "str" = CellText <$> vConverted "string"
| t == "n" = CellDouble <$> vConverted "double"
| t == "n" = CellDecimal <$> vConverted "scientific"
| t == "b" = CellBool <$> vConverted "boolean"
| t == "e" = CellError <$> vConverted "error"
| otherwise = fail "bad cell value"
Expand Down
4 changes: 4 additions & 0 deletions src/Codec/Xlsx/Parser/Internal.hs
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import Data.Maybe
import Data.Text (Text)
import qualified Data.Text as T
import qualified Data.Text.Read as T
import Data.Scientific (Scientific)
import Data.Typeable (Typeable)
import GHC.Generics (Generic)
import Text.XML
Expand Down Expand Up @@ -68,6 +69,9 @@ instance FromAttrVal Integer where
instance FromAttrVal Double where
fromAttrVal = T.rational

instance FromAttrVal Scientific where
fromAttrVal = T.rational

instance FromAttrVal Bool where
fromAttrVal x | x == "1" || x == "true" = readSuccess True
| x == "0" || x == "false" = readSuccess False
Expand Down
5 changes: 5 additions & 0 deletions src/Codec/Xlsx/Parser/Internal/Fast.hs
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ import qualified Data.ByteString.Unsafe as SU
import Data.Char (chr)
import Data.Maybe
import Data.Monoid ((<>))
import Data.Scientific (Scientific)
import Data.Text (Text)
import qualified Data.Text as T
import qualified Data.Text.Encoding as T
Expand Down Expand Up @@ -231,6 +232,10 @@ instance FromAttrBs Double where
-- as for rationals
fromAttrBs = first T.pack . eitherRational . T.decodeLatin1

instance FromAttrBs Scientific where
-- as for rationals
fromAttrBs = first T.pack . eitherRational . T.decodeLatin1

instance FromAttrBs Text where
fromAttrBs = replaceEntititesBs

Expand Down
3 changes: 2 additions & 1 deletion src/Codec/Xlsx/Parser/Internal/PivotTable.hs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import Control.Applicative
import Data.ByteString.Lazy (ByteString)
import Data.List (transpose)
import Data.Maybe (listToMaybe, mapMaybe, maybeToList)
import Data.Scientific (fromFloatDigits)
import Data.Text (Text)
import Safe (atMay)
import Text.XML
Expand Down Expand Up @@ -105,5 +106,5 @@ fillCacheFieldsFromRecords fields recs =
then field {cfItems = mapMaybe recToCellValue recVals}
else field
recToCellValue (CacheText t) = Just $ CellText t
recToCellValue (CacheNumber n) = Just $ CellDouble n
recToCellValue (CacheNumber n) = Just $ CellDecimal (fromFloatDigits n)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not use Scientific in CacheNumber as well?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since I don't understand what CacheNumber is, I trust in your judgement here.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically as far as I remember it's the same thing but for pivot table cache

recToCellValue (CacheIndex _) = Nothing
2 changes: 1 addition & 1 deletion src/Codec/Xlsx/Parser/Internal/Util.hs
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ eitherDecimal t = case T.signed T.decimal t of
rational :: (MonadFail m) => Text -> m Double
rational = fromEither . eitherRational

eitherRational :: Text -> Either String Double
eitherRational :: Fractional a => Text -> Either String a
eitherRational t = case T.signed T.rational t of
Right (r, leftover) | T.null leftover -> Right r
_ -> Left $ "invalid rational: " ++ show t
Expand Down
2 changes: 1 addition & 1 deletion src/Codec/Xlsx/Parser/Stream.hs
Original file line number Diff line number Diff line change
Expand Up @@ -526,7 +526,7 @@ parseValue sstrings txt = \case
string <- maybe (Left $ SharedStringsNotFound idx sstrings) Right $ {-# SCC "sstrings_lookup_scc" #-} (sstrings ^? ix idx)
Right $ CellText string
TStr -> pure $ CellText txt
TN -> bimap (ReadError txt) (CellDouble . fst) $ Read.double txt
TN -> bimap (ReadError txt) (CellDecimal . fst) $ Read.rational txt
TE -> bimap (ReadError txt) (CellError . fst) $ fromAttrVal txt
TB | txt == "1" -> Right $ CellBool True
| txt == "0" -> Right $ CellBool False
Expand Down
17 changes: 14 additions & 3 deletions src/Codec/Xlsx/Types/Common.hs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@
{-# LANGUAGE CPP #-}
{-# LANGUAGE RankNTypes #-}
{-# LANGUAGE GeneralizedNewtypeDeriving #-}
{-# LANGUAGE ViewPatterns #-}
{-# LANGUAGE PatternSynonyms #-}

module Codec.Xlsx.Types.Common
( CellRef(..)
Expand Down Expand Up @@ -39,6 +41,7 @@ module Codec.Xlsx.Types.Common
, xlsxTextToCellValue
, Formula(..)
, CellValue(..)
, pattern CellDouble
, ErrorType(..)
, DateBase(..)
, dateFromNumber
Expand All @@ -51,7 +54,7 @@ module Codec.Xlsx.Types.Common
, _XlsxText
, _XlsxRichText
, _CellText
, _CellDouble
, _CellDecimal
, _CellBool
, _CellRich
, _CellError
Expand All @@ -72,6 +75,7 @@ import Data.Maybe (isJust, fromMaybe)
import Data.Function ((&))
import Data.Ix (inRange)
import qualified Data.Map as Map
import Data.Scientific (Scientific,toRealFloat,fromFloatDigits)
import Data.Text (Text)
import qualified Data.Text as T
import qualified Data.Text.Encoding as T
Expand Down Expand Up @@ -411,12 +415,19 @@ instance NFData Formula
-- - 18.18.11 ST_CellType (Cell Type)
data CellValue
= CellText Text
| CellDouble Double
| CellDecimal Scientific
| CellBool Bool
| CellRich [RichTextRun]
| CellError ErrorType
deriving (Eq, Ord, Show, Generic)

viewCellDouble :: CellValue -> Maybe Double
viewCellDouble (CellDecimal s) = Just (toRealFloat s)
viewCellDouble _ = Nothing

-- view pattern, since 'CellDecimal' has replaced the old constructor.
pattern CellDouble :: Double -> CellValue
pattern CellDouble b <- (viewCellDouble -> Just b)

instance NFData CellValue

Expand Down Expand Up @@ -679,7 +690,7 @@ _CellText
{-# INLINE _CellText #-}
_CellDouble :: Prism' CellValue Double
_CellDouble
= (prism (\ x1_a1ZQy -> CellDouble x1_a1ZQy))
= (prism (\ x1_a1ZQy -> CellDecimal (fromFloatDigits x1_a1ZQy)))
(\ x_a1ZQz
-> case x_a1ZQz of
CellDouble y1_a1ZQA -> Right y1_a1ZQA
Expand Down
2 changes: 1 addition & 1 deletion src/Codec/Xlsx/Types/PivotTable/Internal.hs
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ instance FromCursor CacheField where
cellValueFromNode :: Node -> [CellValue]
cellValueFromNode n
| n `nodeElNameIs` (n_ "s") = CellText <$> attributeV
| n `nodeElNameIs` (n_ "n") = CellDouble <$> attributeV
| n `nodeElNameIs` (n_ "n") = CellDecimal <$> attributeV
| otherwise = fail "no matching shared item"
where
cur = fromNode n
Expand Down
1 change: 1 addition & 0 deletions xlsx.cabal
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,7 @@ Library
, network-uri
, old-locale >= 1.0.0.5
, safe >= 0.3
, scientific
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about adding some reasonable bounds, e.g. >= 0.3.6 (the version where Hashable was fixed)?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we'e only relying on

  • Fractional instance of Scientific,
  • the toRealFloat function

actually a quite low version bound could suffice. 0.3.0.0 appears to be the birth version of toRealFloat.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer the safer bound

, text >= 0.11.3.1
, time >= 1.4.0.1
, transformers >= 0.3.0.0
Expand Down
Loading