API Improvement

[Anarchist666]

Add functions with Map (Int, Int) FormattedCell -> (Worksheet, StyleSheet) and [Map (Int, Int) FormattedCell] -> ([Worksheet], StyleSheet) signatures. Without formatted using.

[qrilka]

Thanks for you contribution @Anarchist666 but I think there is a problem with your approach - you create separate stylesheets for every worksheet and thus you could overwrite results from 1 worksheet with a result from another one but with the same id. Could you take a look into the chaining function from #83 for a more proper way to accomplish what you try to achieve? The main point here is that you shouldn't do formatting independently for every worksheet as they share the same stylesheet. Please let me know if you need any further help with this.

[Anarchist666]

I made some changes. What do you think about? The chaining function uses Formatted/formatted, but I want to create a function without them.

So my idea is to add that and deprecate Formatted/formatted and remove them in the version after the next one.

[Anarchist666]

this function looks to contain a lot of formatted - please either reuse formatted or you could introduce some common helper function though I don't see much sense in that

If you want to remove Formatted/formatted this makes sense.

please don't do manual recursion when the same could be trivially done with a fold

You're a owner and I don't know what you want. Probably, If you fix it than it would be better.

[qrilka]

Unnecessary code duplication isn't good no depending on whether we'll remove formatted or not (I don't see yet any good motivation to do that). Regarding recursion it's one of the most frequent advices, see e.g. Haskell Wiki. And if something's not clear enough - please ask, I will be glad to answer.

[Anarchist666]

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.

And I don't understand why could you need some low level function for multiple sheets and what could it be...

@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

I don't see yet any good motivation to do that

Do you know what you want? I can't do anything before you know this.

Regarding recursion it's one of the most frequent advices, see e.g. Haskell Wiki.

I know, but I think fold with "trivial" constructions for those semantics looks ugly and recursion is more readable. Since you insist, I'll think about it.

[Anarchist666]

Like the chaining function

formatSingleWorksheet :: Map (Int, Int) FormattedCell -> StyleSheet -> (Worksheet, StyleSheet)
formatSingleWorksheet fcs styleSheet = (ws, styleSheet')
  where
    ([ws], styleSheet') = formatWorksheets (fcs:[]) styleSheet

formatWorksheets :: [Map (Int, Int) FormattedCell] -> StyleSheet -> ([Worksheet], StyleSheet)
formatWorksheets fcss initStyle = foldr formatSingle ([], initStyle) fcss
  where
    formatSingle cs (wss, styleSheet) =
       let initSt         = stateFromStyleSheet styleSheet
           (cs', finalSt) = runState (mapM (uncurry formatCell) (M.toList cs)) initSt
           styleSheet'    = updateStyleSheetFromState styleSheet finalSt
           ws             = def & wsCells  .~ (M.fromList (concat cs'))
                                & wsMerges .~ (reverse (finalSt ^. formattingMerges))
       in (ws : wss, styleSheet')

[qrilka]

This last version from the comment resolves my complaint about recursion but the first one about code duplication between formatWorksheets and formatted is still here - could you give me a hint what bothers you about reusing formatted as in https://github.com/qrilka/xlsx/issues/83#issuecomment-301613772 ? With that in place I'd be glad to merge your PR And I'm not against discussing deprecation of formatted but in a separate issue.

[Anarchist666]

ould you give me a hint what bothers you about reusing formatted as in

83 (comment) ?

Obviously #83. Could you read all that issue again, please?

[qrilka]

Obviously what? Could you please explain you thought a bit more explicitly? In my comment I talk about Formatted/formatted being targeted at formatting only 1 worksheet but that doesn't explain why can't it be used for the function formatSingleWorksheet in your PR or for the formatSingle in your comment above (or the other way around - make formatted call formatSingle and deprecate it in a later commit).

[Anarchist666]

and remove them in the version after the next one.

How do you want to use formatted if you wanna remove it? And formatted uses Formatted, but you really wrote you want to remove it too.

In that my comment #83 I offer to change Formatted. But you wrote

@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

Actually it's like trolling. Should I close this request?

[qrilka]

@Anarchist666 as I've said in #83 it's a good practice to keep a deprecated old part of API at least for 1 major release so I don't plan to remove it at least until version 0.9. Excuse me if my explanation were not clear enough. I did not have any intention to troll you, I had just 3 concerns which seemed to me not hard to resolve. Sorry for not explaining things good enough and thank you for your effort.

[Anarchist666]

Can I send a message to e-mail in Russian?

[qrilka]

Конечно :)

[Anarchist666]

I sent a message to gmail