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

Bpmn helper update #111

Merged
merged 15 commits into from
Oct 12, 2023
Merged

Bpmn helper update #111

merged 15 commits into from
Oct 12, 2023

Conversation

LucasMGo
Copy link
Contributor

@LucasMGo LucasMGo commented Oct 5, 2023

Summary

Adapted bpmn-helper module for use in typescript environment. JSDoc comments have been extended and there have been generated declaration files based on these to use in typescript files.

Details

  • Updated bpmn moddle to newest version
  • Improved jsdoc comments
  • Added declaration files for bpmn helper module

@LucasMGo LucasMGo enabled auto-merge October 5, 2023 12:41
Copy link
Contributor

@jjoderis jjoderis left a comment

Choose a reason for hiding this comment

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

In some situations like milestones and callActivities we might expect values to exist when the bpmn is valid. But a bpmn might contain a milestone without a name which is not reflected by the current annotations.
Should every value be set to optional that could have an edge-case where it might not be present even if we expect it to generally exist?

src/helper-modules/bpmn-helper/src/getters.js Outdated Show resolved Hide resolved
src/helper-modules/bpmn-helper/src/getters.js Outdated Show resolved Hide resolved
src/helper-modules/bpmn-helper/src/getters.js Outdated Show resolved Hide resolved
src/helper-modules/bpmn-helper/src/getters.js Outdated Show resolved Hide resolved
src/helper-modules/bpmn-helper/src/getters.js Outdated Show resolved Hide resolved
@OhKai
Copy link
Contributor

OhKai commented Oct 9, 2023

Should every value be set to optional that could have an edge-case where it might not be present even if we expect it to generally exist?

Absolutely, any value that can be optional must be denoted as such. It is the responsibility of the caller to ensure the safe usage of these values by either

  • using optional chaining or checking them against null/undefined and handle exceptions accordingly
  • or if it is 100% semantically clear that the value won't ever be not there (e.g. it was checked earlier and cannot be unset etc.) you can use variable!.method() to force typescript to treat the optional value as non-optional.

src/helper-modules/bpmn-helper/src/setters.js Outdated Show resolved Hide resolved
src/helper-modules/bpmn-helper/src/setters.js Outdated Show resolved Hide resolved
src/management-system-v2/lib/helpers.ts Show resolved Hide resolved
src/management-system-v2/lib/helpers.ts Outdated Show resolved Hide resolved
src/management-system-v2/lib/helpers.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@jjoderis jjoderis left a comment

Choose a reason for hiding this comment

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

LGTM

@LucasMGo LucasMGo merged commit 660d9f4 into main Oct 12, 2023
@LucasMGo LucasMGo deleted the bpmn-helper-update branch October 12, 2023 11:53
FelipeTrost pushed a commit that referenced this pull request Oct 13, 2023
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.

3 participants