implement multiple variable scope #48

[lastchiliarch]

trafficstars

I'm try to implement multiple variable scope in orde to support input variable mapping in local scope.

It contains some break change and has a lot codes, May I ask you to take some time to review ?

Here is the main changes. 

1. implement mulitply variable scope
2. inherit parent scope when job is created
3. introduce local scope
4. modify job complete function  to support variable scope propagation
5. remove embed processInfo from activieJob
6. put input variable mapping into local scope

[codecov[bot]]

Codecov Report

Merging #51 (c91da2e) into main (bd9cb24) will increase coverage by 0.88%. The diff coverage is 95.16%.

@@            Coverage Diff             @@
##             main      #51      +/-   ##
==========================================
+ Coverage   74.34%   75.23%   +0.88%     
==========================================
  Files          20       21       +1     
  Lines        1033     1070      +37     
==========================================
+ Hits          768      805      +37     
  Misses        246      246              
  Partials       19       19              

Impacted Files	Coverage Δ
pkg/bpmn_engine/variable_scope/scope.go	92.85% <92.85%> (ø)
pkg/bpmn_engine/engine.go	90.67% <100.00%> (-0.11%)	:arrow_down:
pkg/bpmn_engine/engine_events.go	86.66% <100.00%> (ø)
pkg/bpmn_engine/engine_jobs.go	100.00% <100.00%> (ø)
pkg/bpmn_engine/engine_jobs_activated.go	100.00% <100.00%> (ø)
pkg/bpmn_engine/engine_process_instance.go	66.66% <100.00%> (+1.66%)	:arrow_up:
pkg/bpmn_engine/expressions.go	100.00% <100.00%> (ø)

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

[nitram509]

Looks good at a first brief scan of mine. I plan to review & merge by next week due to some private obligations.

[lastchiliarch]

Looks good at a first brief scan of mine. I plan to review & merge by next week due to some private obligations.

Thank you very much.

[nitram509]

Hi @lastchiliarch I found the issue with altering the variable context when job are in flight, see #55 I try to fix this by tomorrow, so you can merge it into your PR.

[nitram509]

@lastchiliarch I took a bit longer than expected, but the interface is now cleaned up.

[lastchiliarch]

@lastchiliarch I took a bit longer than expected, but the interface is now cleaned up.

Thanks for you help. I'm working on this.

[lastchiliarch]

I have removed local scope and rebase the code to main, could you please spend some time to review it again? @nitram509

[lastchiliarch]

I have removed local scope and rebase the code to main, could you please spend some time to review it again? @nitram509

Seems it takes a long time on this pr, could you have a look at this again? If anything need improve, I will work on this again. @nitram509

[nitram509]

Hi @lastchiliarch I'm sorry for the long pause. I will take a look at your latest updates over the upcoming days.

[nitram509]

Hi @lastchiliarch I did review your PR and took your good ideas one step further. I wanted to simplify the variable scope/context to just a unified implementation, not two 'VarScope' implementations. With this unified implementation, the name changed to VariableHolder. Also, I wanted the VariableHolder to be self-contained as much as possible, following encapsulation principles. That means, the holder knows, what propagating means and does the copies on its own.

Here's what I came up with: https://github.com/nitram509/lib-bpmn-engine/pull/68/files The implementation is shorter.

After all, I found it difficult to merge the meanwhile evolved main branch. So, I will close this PR and cherry-pick to main manually.

Thank you very much for your patience and support.

[lastchiliarch]

Hi @lastchiliarch I did review your PR and took your good ideas one step further. I wanted to simplify the variable scope/context to just a unified implementation, not two 'VarScope' implementations. With this unified implementation, the name changed to VariableHolder. Also, I wanted the VariableHolder to be self-contained as much as possible, following encapsulation principles. That means, the holder knows, what propagating means and does the copies on its own.

Here's what I came up with: https://github.com/nitram509/lib-bpmn-engine/pull/68/files The implementation is shorter.

After all, I found it difficult to merge the meanwhile evolved main branch. So, I will close this PR and cherry-pick to main manually.

Thank you very much for your patience and support.

I have reviewd the new implementation, it's more elegant.
Thanks for your help to make this feature achieved. By the way, I was wondering if this project was inactived. Glad to see you are still working on this project.