Cleanup #158
Cleanup #158
Conversation
…ld never be nil, so the if statement was basically dead code. Removed some cruft afterwards.
| @@ -124,10 +125,5 @@ func findExistingTimerNotYetTriggered(state *BpmnEngineState, id string, instanc | |||
| } | |||
|
|
|||
| func findDurationValue(ice BPMN20.TIntermediateCatchEvent) (duration.Duration, error) { | |||
| durationStr := &ice.TimerEventDefinition.TimeDuration.XMLText | |||
| if durationStr == nil { | |||
| return duration.Duration{}, newEngineErrorf("Can't find 'timeDuration' value for INTERMEDIATE_CATCH_EVENT with id=%s", ice.Id) | |||
There was a problem hiding this comment.
durationStr could never be nil, so that error never actually occured.
There was a problem hiding this comment.
@GodsBoss good catch, if durationStr can't be nil - but then we need to rewrite the if clause, to capture the case when the text is empty. The goal here is to capture required and missing input as early as possible and provide good error message to the user.
If you agree, would you please bring the check back?
There was a problem hiding this comment.
@nitram509 I'd rather have this PR only change cosmetics, not behaviour. If you wish I can remove that particular change.
There was a problem hiding this comment.
My point was rather to rework this change towards a fail fast check and not removing "dead code".
Don't worry, I will merge it and then bring back the change on my own ... that round trip is shorter.
|
As spotted by GodsBoss, there is no message error issue. |
Codecov Report
@@ Coverage Diff @@
## main #158 +/- ##
==========================================
+ Coverage 64.09% 64.48% +0.38%
==========================================
Files 24 24
Lines 1557 1543 -14
==========================================
- Hits 998 995 -3
+ Misses 534 524 -10
+ Partials 25 24 -1
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| @@ -124,10 +125,5 @@ func findExistingTimerNotYetTriggered(state *BpmnEngineState, id string, instanc | |||
| } | |||
|
|
|||
| func findDurationValue(ice BPMN20.TIntermediateCatchEvent) (duration.Duration, error) { | |||
| durationStr := &ice.TimerEventDefinition.TimeDuration.XMLText | |||
| if durationStr == nil { | |||
| return duration.Duration{}, newEngineErrorf("Can't find 'timeDuration' value for INTERMEDIATE_CATCH_EVENT with id=%s", ice.Id) | |||
There was a problem hiding this comment.
@GodsBoss good catch, if durationStr can't be nil - but then we need to rewrite the if clause, to capture the case when the text is empty. The goal here is to capture required and missing input as early as possible and provide good error message to the user.
If you agree, would you please bring the check back?
|
@GodsBoss thank you for your contribution 🚀 |
Motivation/Abstract
There were some cosmetic issues and also some dead code. Imports weren't formatted with
goimports, for example.Description/Comments
Imports are now formatted with
goimports, did a few other changes that keep the behaviour of the code. I did not create an issue because that seemed unnecessarely bureaucratic to me.Checklist
Depending on your PR, please ensure the overall consistency/integrity of the project remains.
Please tick just one check item per section below
Tests
Code examples
Documentation