nfdi4plants / ARCCommander

Tool to manage your ARCs
MIT License
11 stars 9 forks source link

Why the 'normal' and 'sst' version of value access functions in FSharpSpreadsheetML? #22

Closed kMutagene closed 3 years ago

kMutagene commented 3 years ago

Many functions have a normal and sst version, where the difference is that the sst versions check if the cell(s) are of type SharedString

 let getValueWithSST (sharedStringTable:SharedStringTable) (cell:Cell) =
        match cell |> tryGetType with
        | Some (CellValues.SharedString) ->

            let sharedStringTableIndex = 
                cell
                |> getValue
                |> int

            sharedStringTable
            |> SharedStringTable.getText sharedStringTableIndex
            |> SharedStringItem.getText
        | _ ->
            getValue cell

Is there a reason to now have so many sst versions of functions? The value access is guarded anyways, and will return the 'normal' cell value anyways if the CellValue is not SharedString. If this is not necessary, i would suggest to just always work with the sst versions and remove the 'normal' ones altogether to reduce namespace noise

HLWeil commented 3 years ago

That would surely increase the usability. The thing is, how would you then fill the input paramet sharedStringTable if you don't have a SharedStringTable in your spreadsheet. I think in that case the input parameters should be altered a little. sharedStringTable could be replaced by workbookPart or something and internally the existance of a SharedStringTable could be checked.

kMutagene commented 3 years ago

The thing is, how would you then fill the input paramet 'sharedStringTable' if you don't have a SharedStringTable in your spreadsheet

Is that guarded at the moment? All instances of usage of SST functions in the ISAXLSX library use an unsafe access via WorkbookPart.getSharedStringTable anyways.

example:

/// If the row contains a key and at least one value, returns them
    let tryParseKeyValues workbookPart row =
        let sst = workbookPart |> WorkbookPart.getSharedStringTable
        row
        |> Row.getIndexedValuesWithSST sst
        |> fun s -> 
            match Seq.tryFind (fst >> (=) 1u) s with
            | Some (_,k) -> 
                KeyValuePair(k,Seq.skip 1 s) |> Some                                     
            | _ -> None

As the intended use of the library is to work from the top level, where there is access to the shared string table anyways, the table is accessible via WorkbookPart.getOrInitSharedStringTablePart even if it does not exist, and can be passed down to the internal API calls.

When users want to use low level APIs, i think it is reasonable to keep the sharedStringTable parameter, or maybe use an optional parameter here, that defaults the functions to return the normal cell values.

HLWeil commented 3 years ago

In principle, a guarding could be introduced with a tryGetSharedStringTable function but actually I think using an optional sharedStringTable is a better approach as it's more transparent to the user.

kMutagene commented 3 years ago

Allright 👌 , will include this in my refactor attempts

HLWeil commented 3 years ago

Closing this as this was fixed in db520f1