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

Feature; excel; create and update input and output columns #482

Merged
merged 6 commits into from
Jul 16, 2024

Conversation

Etschbeijer
Copy link
Collaborator

Closes Issue #470
Enable the addition of new input- and output columns and also update existing ones, when they are already available

Enable update of input column
@Etschbeijer Etschbeijer self-assigned this Jul 15, 2024
@Etschbeijer Etschbeijer changed the base branch from main to developer July 15, 2024 09:33
@Etschbeijer Etschbeijer marked this pull request as ready for review July 15, 2024 09:33
| Some Swatehost.Browser | Some Swatehost.ARCitect ->
let cmd = Spreadsheet.AddAnnotationBlock minBuildingBlockInfo |> SpreadsheetMsg |> Cmd.ofMsg
model, cmd
| _ -> failwith "not implemented"
| AddAnnotationBlocks minBuildingBlockInfos ->
| AddAnnotationBlocks compositeColumn ->
Copy link
Collaborator

Choose a reason for hiding this comment

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

compositeColumns

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

checkIfBuildingBlockExisting newBB existingBuildingBlocks
// if newBB is output column and output column already exists in table this returns (Some outputcolumn-building-block), else None.
let outputColOpt = checkHasExistingOutput newBB existingBuildingBlocks
let oldInputColumn = annotationTable.columns.items.[(int) inputColumn.Value.index].name
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is inputColumn.Value.name?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed


let rowCount = arcTable.RowCount + 1

let createCol name =
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe make this generic and reuseable for other functions

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

src/Client/OfficeInterop/OfficeInterop.fs Show resolved Hide resolved
annotationTable.Value.columns.load(propertyNames = U2.Case2 (ResizeArray[|"items"; "name"; "values"; "index"; "count"|]))

let! result = context.sync().``then``(fun _ ->
if newBB.Header.isInput then
Copy link
Collaborator

Choose a reason for hiding this comment

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

active pattern instead of if-else

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

@@ -51,6 +51,14 @@ module OfficeInteropExtensions =
|> Seq.map (snd >> Seq.map snd >> Seq.toArray)
|> Seq.toArray

static member addCollumn (annotationTable:Table) name rowCount value =
Copy link
Collaborator

Choose a reason for hiding this comment

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

Column*

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

@@ -135,6 +143,38 @@ module OfficeInteropExtensions =
else return None
}

static member tryGetFromExcelTable (annotationTable:Table, context:RequestContext) =
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe rename "annotationTable" everywhere to "excelTable" or "eTable" for more clarity.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

let! result = context.sync().``then``(fun _ ->
if newBB.Header.isInput then

let (|Input|_|) (newBuildingBlock:CompositeColumn) =
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not sure if these are really helpful.

I had something like this in mind. Which can be used as a generic helper for future functions:

/// This is a mockup
type ExcelTable =
  abstract member GetHeaderIndex: string -> int

/// This returns excel table index for the input column
let (|Input|_|) (arctable: ArcTable) (excelTable: ExcelTable) =
  match arctable.TryGetInputColumn() with
  | Some c -> 
    let headerName = c.ToString()
    excelTable.GetHeaderIndex(headerName)
    |> Some
  | _ -> None

/// This returns excel table index for the input column
let (|Ouput|_|) (arctable: ArcTable) (excelTable: ExcelTable) =
  match arctable.TryGetOutputColumn() with
  | Some c -> 
    let headerName = c.ToString()
    excelTable.GetHeaderIndex(headerName)
    |> Some
  | _ -> None


// opened an issue for this: https://github.com/nfdi4plants/ARCtrl/issues/416
let (|Unique|_|) (header:CompositeHeader) (arctable: ArcTable) (excelTable: ExcelTable) =
  match (header.IsFeaturedColumn || header.IsSingleColumn) with
  | true -> 
    match arctable.TryGetColumnByHeader header with
    | Some c -> 
      let headerName = c.ToString()
      excelTable.GetHeaderIndex(headerName)
      |> Some
    | _ -> None
  | _ -> None

Copy link
Collaborator Author

@Etschbeijer Etschbeijer Jul 15, 2024

Choose a reason for hiding this comment

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

Ok, shall I implement the generic method in this issue or not?
It could be better when I finish the different single building blocks first and create the general tool at the end. Then I have a better overview of the required data and so.

Copy link
Collaborator

Choose a reason for hiding this comment

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

As you see fit. But the current implementation of let (|Input|_|) (newBuildingBlock:CompositeColumn) =, let (|Output|_|) (newBuildingBlock:CompositeColumn) = seems more complicated than it helps.

let updateInputColumn (excelTable:Table) (arcTable:ArcTable) (newBB:CompositeColumn) =

let possibleInputColumn = arcTable.TryGetInputColumn()
let inputColumnName =
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do not default to column name to "" if it is empty, just work with options. No reason to run the code below if we get a None here.

Option.map (fun c -> c.Header.ToString())
|> Option.map (fun cName -> excelTable.columns.item.Find (fun s -> ...)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Same for output

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

let annotationTable = sheet.tables.getItem(annotationTableName)
//let activeSheet = context.workbook.worksheets.getActiveWorksheet()
//Try to get the name of the currently active sheet
let! excelTableName = tryGetActiveAnnotationTableName context
Copy link
Collaborator

Choose a reason for hiding this comment

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

fail if this isNone, then shadow it with its own .Value

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

let! result = context.sync().``then``(fun _ ->
if newBB.Header.isInput then

let (|Input|_|) (newBuildingBlock:CompositeColumn) =
Copy link
Collaborator

Choose a reason for hiding this comment

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

As you see fit. But the current implementation of let (|Input|_|) (newBuildingBlock:CompositeColumn) =, let (|Output|_|) (newBuildingBlock:CompositeColumn) = seems more complicated than it helps.

@Etschbeijer Etschbeijer merged commit a4461e7 into developer Jul 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants