qrilka/xlsx

Using 'formatted' with multiple sheets

Closed this issue · 20 comments

Hi!

With 'formatted' it is only possible to generate indexed styles for an xlsx with a single sheet because the input CellMap does not carry any information about the sheet a cell belongs to - CellMap is indexed by (row, column).

Applying 'formatted' separately for each sheet's cells result in one StyleSheet per sheet. However only one style sheet can be rendered and set for the xlsx and cells can only refer to a "global" index.

Did I overlook something or is this a limitation of the current 'formatted' implementation?

Yeah, you are right and formatted is worksheet-oriented. By itself it's kinda workaround for not doing that work in writing and parsing and was made by @edsko on top of existing code providing raw style ids. I wish I could find some time to provide more high level API :)
But could use a workaround just pass StyleSheet between formatted output and its next input.
A PR adding such a helper doing this will be highly appreciated.

Thanks a lot for your comments - seems I was actually blind. Accumulating the style sheets with some foldr logic does exactly what I need.

I'm not sure it makes sense to offer a helper function doing that because the whole cell building process most likely depends on the specific use case.

However adding a simple example that builds an xlsx containing two sheets and some simple formatting with 'formatted' would propably be a good idea. Would you accept such an PR?

For sure I'm open for a discussion :)

stla commented

👍
I came here for opening such an issue. I didn't think to the possibility of chaining.
I'm thinking about a function
[Map (Int, Int) FormattedCell] -> (StyleSheet, [Worksheet])

stla commented

This seems to work. I'm not an expert in Haskell, maybe the code can be simplified.

chaining :: [FormattedCellMap] -> (StyleSheet, [Worksheet])
chaining fcellmaps =
  case length fcellmaps of
    1 -> (stylesheet, [ws])
      where ws = set wsMerges (formattedMerges frmt) $
                   set wsCells (formattedCellMap frmt) emptyWorksheet
            stylesheet = formattedStyleSheet frmt
            frmt = formatted (head fcellmaps) minimalStyleSheet
    _ -> (stylesheet, snd previous ++ [ws])
      where ws = set wsMerges (formattedMerges frmt) $
                   set wsCells (formattedCellMap frmt) emptyWorksheet
            stylesheet = formattedStyleSheet frmt
            frmt = formatted fcellmap (fst previous)
            previous = chaining (take (l-1) fcellmaps)
            fcellmap = last fcellmaps
            l = length fcellmaps

@stla I haven't tested it but could this done with this shorter snippet?

chaining :: [M.Map (Int, Int) FormattedCell] -> ([Worksheet], StyleSheet)
chanining fWss = foldr formatSingle ([], minimalStyleSheet) fWss
  where
    formatSingle fcells (wss, ssheet) =
       let fmt = formatted fcells ssheet
           ws = def & wsCells .~ formattedCellMap fmt
                    & wsMerges .~ formattedMerges fmt
       in (ws : wss, formattedStyleSheet fmt))

BTW what's FormattedCellMap in your code? I suppose M.Map (Int, Int) FormattedCell?

stla commented

Yes, your clean code works. Thanks ! You can even remove fWss.
Yes, you have the right guess about FormattedCellMap.

If you just add chaining as formattedSheets, then why Formatted will be needed? formatted function creates the Formatted, but in chaining Formatted not used. Probably, in this case formatted is necessary for use private updateStyleSheetFromState. Either you need to change Formatted for support multiple sheet, which is probably very difficult.

@Anarchist666 have you seen by 1st answer in this topic about the origin of Formatted module?
Overall you're right and with this helper Formatted record won't be strictly required.
But even having that I'd like to keep code at least backward compatible and I'm not 100% sold that this helper could cover all use cases of formatted+Formatted.

I think, Formatted and formatted is high level API, but chaining :: [M.Map (Int, Int) FormattedCell] -> ([Worksheet], StyleSheet) is low level API. Using formatted from high level in chaining looks like dirty hack. If high level API not ready yet, may be create low level function for supporting multiple sheets?

How could formatted and Formatted be high level API when it's just intermediate step to get worksheets with formatting and corresponding stylesheet? And I don't understand why could you need some low level function for multiple sheets and what could it be...

Ok, i understand that the Map (Int, Int) FormattedCell is a part of high level API. If _wsCells :: CellMap and , _wsMerges :: [Range] is a part of Worksheet then may be made:

data Formatted = Formatted { worksheets :: [Worksheet] , formattedStyleSheet :: StyleSheet } deriving (Eq, Show, Generic)

That sounds like a good idea but as I've said - I'm not (yet?) totally convinced that we could just get rid of previous formatted+Formatted. Also this will be a non backwards compatible change.
@njeisecke @stla do you have any thoughts on such an idea?

I'm fine with the example given by @qrilka which can easily be adapted to everyone's need. All API is here and ready to be used. If that example had been part of the docs I would not have raised this issue at all.

Оne of the options:

data Formatted = Formatted {
    worksheets :: [Worksheet]
  , formattedStyleSheet :: StyleSheet
} deriving (Eq, Show, Generic)

formattedCellMap :: Formatted -> CellMap
formattedCellMap = _wsCells . head . worksheets

formattedMerges :: Formatted -> [Range]
formattedMerges = _wsMerges . head . worksheets

But mark as depricated.

@Anarchist666 I don't quite understand why could we need this. The current Formatted was designed to be used with formatted which works for 1 sheet. With formatSheets (named chaining above) imo it looks OK to have just ([Worksheet], StyleSheet) as a return value. So my idea is to add that and deprecate Formatted/formatted and remove them in the version after the next one.
If there will be no PRs and no better proposals that will be my plan after I finish work on #100

I thought you were worried about backward compatibility.

Keeping but deprecating old API for 1 release is normal practice, do you see any issue with it?

But even having that I'd like to keep code at least backward compatible and I'm not 100% sold that this helper could cover all use cases of formatted+Formatted.

Apparently I misunderstood you

Resolved by #119