-
Notifications
You must be signed in to change notification settings - Fork 30
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
Explicit extension namespaces #45
Draft
nikku
wants to merge
6
commits into
main
Choose a base branch
from
extension-namespace-foo
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nikku
force-pushed
the
extension-namespace-foo
branch
from
December 17, 2022 16:38
b209946
to
733e24d
Compare
Merged
nikku
force-pushed
the
extension-namespace-foo
branch
from
December 17, 2022 20:46
6f9c804
to
8a65475
Compare
bpmn-io-tasks
bot
added
in progress
Currently worked on
and removed
needs review
Review pending
labels
Dec 18, 2022
nikku
force-pushed
the
extension-namespace-foo
branch
from
December 20, 2022 13:59
8a65475
to
a6ec0af
Compare
nikku
force-pushed
the
extension-namespace-foo
branch
from
December 20, 2022 16:06
a6ec0af
to
1885ef9
Compare
nikku
force-pushed
the
extension-namespace-foo
branch
from
December 21, 2022 21:10
1885ef9
to
0b20cb6
Compare
nikku
force-pushed
the
extension-namespace-foo
branch
from
December 21, 2022 21:38
6536c1f
to
ff79b75
Compare
This was referenced Feb 14, 2023
nikku
force-pushed
the
extension-namespace-foo
branch
from
February 23, 2023 13:49
ff79b75
to
39a120a
Compare
This resolves name conflicts by making extensions only available through their namespace names. Related to #36 BREAKING CHANGES: * Extensions are not registered with their local names anymore, but only via their namespace prefixed names such as `someProperty` -> `e:someProperty`
Outlines solution to #36
nikku
force-pushed
the
extension-namespace-foo
branch
from
February 24, 2023 09:02
6a53689
to
07f8e2a
Compare
This issue is also preventing us to create a generic bpmn-linter that support both I verified this is the same issue with a test-case on https://github.com/bpmn-io/moddle/tree/test-allow-overlapping-types |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Context
This PR fixes a long standing issue that prevents general usability of moddle for advanced use-cases, i.e. UML parsing: We handle extension provided properties in a magic fashion, and polute the local namespace with them. All under the assuption that names declared across packages do not ever overlap (which can only hold true accidentally). In case of overlap we blow up (#36).
UML specifically defines two packages
UML
andUMLDI
, and both declare a propertyownedElement
:(Image from #36).
The Solution
This PR makes extension namespaces explicit, offering a more predicatable
moddle
behavior, and enabling use-cases such as #36 in the first place:Remove magic access to non-namespaced names for extension properties:
As a result you may define properties with name clashes in a safe manner:
Regarding the UML Case
For ease of use in
moddle
users must clearly separate local from extension properties. Inheritance must still be conflict free, meaning that the stock UML case ⬆️ is still not suppported out of the box. However you could argue that introducing such conflicts in the first place is bad language design. We have no urge to support such.Given this PR you can however solve the situation through a virtual glue package (8a65475). That glue package does what UML should have done in the first place, clearly defining ownership (core domain - UMLDI) and extension (UML):
As indicated earlier this is a major breaking change: Where extension properties have been mapped via their local names in the past, they now must be mapped explicitly.
When considering the core bpmn.io domain this results in minor adjustments (f058e3a, bpmn-io/moddle-xml#68, bpmn-io/bpmn-moddle#97). When we considering the extension domain, this results in major changes (camunda/camunda-bpmn-moddle#122, camunda/zeebe-bpmn-moddle#40).
Migration
To aid migration migration to this new major version (and guide our users through stricter usage requirements) I suggest a
strict
mode and explicit logging of miss-use (#48).