Skip to content

fix(ids): require every applicability facet to match - #814

Open
helenkwok wants to merge 1 commit into
ThatOpen:mainfrom
helenkwok:fix/ids-applicability-intersection
Open

helenkwok wants to merge 1 commit into
ThatOpen:mainfrom
helenkwok:fix/ids-applicability-intersection

Conversation

@helenkwok

Copy link
Copy Markdown
Contributor

Description

Fixes #813.

IDSSpecification.test() now gives each applicability facet its own collector and intersects the results with the existing ModelIdMapUtils.intersect. So an element is applicable only when it matches every applicability facet, as IDS defines it. Before, all facets wrote into one shared map, so the result was their union.

Additional context

  • New specification.test.ts uses the existing tests/fixtures/type-psets.ifc and the same headless setup as facets/property.test.ts. "Walls that are external" now tests walls 40, 41, 42 and 46. Without the change it tests all six (expected [40, 41, 42, 43, 45, 46] to deeply equal [40, 41, 42, 46]). A single facet still selects what it did.
  • vitest --run in packages/core: 59 of 59 pass. tsc -p tsconfig-build.json --noEmit is clean.
  • One behaviour to be aware of: a facet whose getEntities selects nothing now makes the whole applicability select nothing. That matters for the Attribute facet, whose getEntities is currently empty, and for Classification and PartOf, which are stubs. Under the union those facets were silently ignored; with the intersection, a specification that uses them tests no elements until they are implemented. Both are wrong today in different ways. I can open separate issues for those facets if that helps.

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: applicability facets are combined with OR, so a specification tests elements that match only one of them

1 participant