Systemorph / IFRS17CalculationEngine

Systemorph IFRS 17 Calculation Engine
MIT License
28 stars 5 forks source link

Conclude discussion on interactive object PR #217 #248

Closed dcolleoni closed 1 year ago

dcolleoni commented 1 year ago

Here I copy some comments to PR #217 which could not addressed within the PR.

ReportMutableScopeInteractive line 95 : What is the advantage of defining a singletone having only a workspace property (it should be with capital W) that can be accessed easily through the storage? IMO this makes the code harder to read because it would not be immediately clear where this workspace comes from from reading the subsequent scopes.

ReportMutableScopeInteractive line 150 : why is this not taken from the storage? it would be more efficient than querying each time. See also https://github.com/Systemorph/IFRS17CalculationEngine/issues/237.

ReportMutableScopeInteractive line 527 : It is not immediately clear to me what this multithreading issue is about, but I think I have an idea. It might be that this workspace here is one of the source of the issue, because all dropdowns also use to query this workspace (which eventually query the datasource) and the controls happen to be on different concurrent tasks. If the dropdowns used the storage without query to the workspace (as I suggest in the comments above), then the query performed here to produce the csv could just work ok.

andrey-katz-systemorph commented 1 year ago

Here is the summary of further discussions and investigation.

  1. As discussed, the advantage of this way of coding is in possible use in subsequent projects where the storages inherit from the generic storage. The names of the variables and capitalization are in agreement with the standard naming of c# variables (it is a protected variable, not public).
  2. After discussion with @amuolo we decided that this would be a separate ticket on the board, that would include: develop a method of the report storage that would query dimensions, cache them into dictionaries, and retrieve them (alongside the current GetAutocomplete which is an extension method of the workspace). This would reduce the number of times we query the DB (#254)
  3. I tried the idea of manually locking the thread at the stage of storage initialization in each of the methods where it is required, explicitly or implicitly. It does not hurt, but it does not help either. Given these not encouraging results, I would propose to leave this topic until it is properly taken care of at the backend.

So, I believe with this discussion can be closed @amuolo , @dcolleoni

dcolleoni commented 1 year ago

Thanks @andrey-katz-systemorph, As far as I am concerned, we can close this.

@andrey-katz-systemorph pls close the issue when it's done. Thanks

andrey-katz-systemorph commented 1 year ago

Thanks, I am moving this ticket to done.

FYI: @amuolo @dcolleoni