Skip to content

Remove OptionalPlaceHolders on NodeState Instances#3573

Merged
mrsuciu merged 4 commits into
OPCFoundation:master378from
mrsuciu:noOptionalPlaceholders
May 13, 2026
Merged

Remove OptionalPlaceHolders on NodeState Instances#3573
mrsuciu merged 4 commits into
OPCFoundation:master378from
mrsuciu:noOptionalPlaceholders

Conversation

@mrsuciu
Copy link
Copy Markdown
Contributor

@mrsuciu mrsuciu commented Feb 20, 2026

Proposed changes

Test that Optional place holders are not present in node state instancesNo optional placeholderse.

Related Issues

  • Fixes #

Types of changes

What types of changes does your code introduce?
Put an x in the boxes that apply. You can also fill these out after creating the PR.

  • Bugfix (non-breaking change which fixes an issue)
  • Enhancement (non-breaking change which adds functionality)
  • Test enhancement (non-breaking change to increase test coverage)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected, requires version increase of Nuget packages)
  • Documentation Update (if none of the other choices apply)

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

  • I have read the CONTRIBUTING doc.
  • I have signed the CLA.
  • I ran tests locally with my changes, all passed.
  • I fixed all failing tests in the CI pipelines.
  • I fixed all introduced issues with CodeQL and LGTM.
  • I have added tests that prove my fix is effective or that my feature works and increased code coverage.
  • I have added necessary documentation (if appropriate).
  • Any dependent changes have been merged and published in downstream modules.

Further comments

If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...

@marcschier marcschier marked this pull request as draft April 14, 2026 06:49
private static bool IsPlaceholderChild(BaseInstanceState child)
{
if (child.ModellingRuleId == ObjectIds.ModellingRule_OptionalPlaceholder ||
child.ModellingRuleId == ObjectIds.ModellingRule_MandatoryPlaceholder)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why remove MandatoryPlaceholder?

Copy link
Copy Markdown
Contributor Author

@mrsuciu mrsuciu May 6, 2026

Choose a reason for hiding this comment

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

Because it is still a place holder declaration not a concrete runtime instance that should not appear as a concrete child in a runtime instance.

Copy link
Copy Markdown
Contributor Author

@mrsuciu mrsuciu May 6, 2026

Choose a reason for hiding this comment

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

However commits from f673061 upto 699fbe7 are wrong leftovers and need to be removed

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@mrsuciu okay, so this restores the behaviour from before 1.5.378? Because Else i would think it could Break Adress spaces of Users that expect those placeholders

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@marcschier whats your opinion in that?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@romanett These placeholders should not exist on the instances, just on the declarations which are part of Types

@mrsuciu mrsuciu force-pushed the noOptionalPlaceholders branch from 94bc964 to bd2c63d Compare May 6, 2026 10:39
@mrsuciu mrsuciu marked this pull request as ready for review May 7, 2026 14:23
@romanett romanett changed the title Test that Optional place holders are not present in node state instancesNo optional placeholders Remove OptionalPlaceHolders on NodeState Instances May 13, 2026
@mrsuciu mrsuciu merged commit 9d43d12 into OPCFoundation:master378 May 13, 2026
50 checks passed
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.

2 participants