Skip to content

fix(ids): apply optional and prohibited cardinality to the property facet as a whole - #822

Open
helenkwok wants to merge 2 commits into
ThatOpen:mainfrom
helenkwok:fix/ids-property-cardinality
Open

helenkwok wants to merge 2 commits into
ThatOpen:mainfrom
helenkwok:fix/ids-property-cardinality

Conversation

@helenkwok

@helenkwok helenkwok commented Oct 11, 2026 •

Copy link
Copy Markdown
Contributor

Description

Fixes #821.

IDSProperty.test now evaluates the facet itself as if it were required (property set, name, value and data type), in a new private evalFacet. It then applies the cardinality once:

Cardinality Result
required the detailed checks, as before
prohibited one BaseName check that passes when the facet does not match
optional one passing BaseName check, whatever the outcome (IDS developer guide: an optional requirement that would fail if required must not be logged as an error; test cases property/pass-an_optional_facet_always_passes_regardless_of_outcome_*)

IDSFacet.evalRequirement gains an optional cardinality argument, defaulting to the facet's own, so evalFacet can match names, values and data types without the cardinality being applied to each one.

Additional context

  • New facets/property-cardinality.test.ts on tests/fixtures/type-psets.ifc: required with a value; prohibited without a value (only the wall without the property passes); optional (all pass, including the wall with no Pset_WallCommon). The prohibited and optional cases fail without the change. The existing property.test.ts still passes.
  • Against buildingSMART's IDS implementer test cases, the optional and prohibited property cases give the expected results with this change.
  • The IDS user manual describes optional as "if present, it needs to have the value", while the developer guide says such a failure must not be logged as an error. With only pass/fail available, this follows the developer guide; a warning would fit the manual better if you'd rather add one.
  • vitest --run in packages/core: all pass. tsc -p tsconfig-build.json --noEmit is clean.
  • Not changed here: the other facets (Attribute, Material, Classification) also apply cardinality inside evalRequirement per check. They may have similar issues, but I haven't checked them.

What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following:

  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Follow the Conventional Commits v1.0.0 standard for PR naming (e.g. feat(examples): add hello-world example).
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

This branch has not been deployed

No deployments
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.

IDS: optional and prohibited property requirements give inverted results

1 participant