fix(core): IFCELECTRICDISTRIBUTIONPOINT was spelled IFCELECTRICAL- in four places - #3185
fix(core): IFCELECTRICDISTRIBUTIONPOINT was spelled IFCELECTRICAL- in four places#3185BIMvoice wants to merge 2 commits into
Conversation
…erywhere `IfcElectricDistributionPoint` is a real, concrete IFC2X3 entity (`IfcFlowController`, carrying ObjectPlacement and Representation). Every place that claimed to handle it spelled it `IfcElectricalDistributionPoint`, with an "AL" no IFC schema has. Four sites, two languages: - rust/core/src/legacy_entities.rs: the map key, so the arm could never match a real file and the entity fell through to IfcType::from_str -> Unknown, losing the has_geometry: true that arm exists to give it. The meshing pass then skipped it silently. - rust/core/src/schema_helpers_tests.rs: asserted has_geometry_by_name on the misspelling. It passed -- against the table key that was wrong the same way. - packages/parser/src/ifc-schema.ts: ENTITY_NAME_ALIASES carried the same misspelled key, its own comment saying it mirrors legacy_entities.rs. It did mirror it, typo included. - packages/parser/test/known-type-across-schemas.test.ts: asserted the misspelling resolves, describing it as "real, deprecated IFC2X3 syntax". It is not syntax at all. So the two sides were in lockstep and both tests were green, on a name that does not exist, while the entity that does exist got nothing. This is why "the tests pass" was not evidence here: the oracle shared the defect. Verified both directions on the Rust side: cargo test -p ifc-lite-core --lib -> 161 passed, 0 failed. Restoring the typo fails electric_distribution_point_is_spelled_as_the_step_keyword with "IFCELECTRICDISTRIBUTIONPOINT is a real IFC2X3 keyword and must resolve". The new tests assert the misspelling is ABSENT as well as the real name present, so re-adding it as an extra arm cannot quietly satisfy them. Two things deliberately NOT done: The TS alias entry is kept rather than deleted, though on that side it is redundant: ENTITIES_IFC2X3 carries IfcElectricDistributionPoint, so getInheritanceChain reaches IfcDistributionElement through its real parents (IfcFlowController -> IfcDistributionFlowElement -> IfcDistributionElement) without any alias. The alias exists to stay in lockstep with the Rust core, whose generated enum is IFC4X3-only and genuinely needs it. The comment there claimed no bundled table carries the entity, which was false for the corrected name; that premise is now fixed too. base_type stays IfcDistributionElement. The real parent is IfcFlowController, which does exist in the Rust schema and would be more precise, but changing it changes classification for a live entity and is a separate decision from repairing a name. TS side is not verified locally: this worktree has no node_modules and the one that had them lost its .git to an unrelated prune. The TS change is a key rename plus the matching test, and the parent chain above was read from the bundled table rather than assumed. CI runs those tests. Refs #3172, which reported this typo alongside a larger gap in the same table that needs a mapping decision and is not touched here.
|
Warning Review limit reachedNext included review available in 20 minutes. View limit detailsLimit details: You’ve used all 2 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Viewer benchmark✅ No threshold regressions detected. 01_Snowdon_Towers_Sample_Structural(1).ifcBaseline recorded 2026-07-01T20:31:05.538Z on github-actions ubuntu-latest, viewer-benchmark-ci (headless Chrome, SwiftShader ANGLE), production build.
AC20-FZK-Haus.ifcBaseline recorded 2026-07-01T20:30:59.972Z on github-actions ubuntu-latest, viewer-benchmark-ci (headless Chrome, SwiftShader ANGLE), production build.
Refresh the baseline from a CI run: dispatch the Benchmark workflow with |
CI caught what I could not run locally. packages/query's
oftype-unknown-type.test.ts asserted that ofType('IfcElectricalDistributionPoint')
does not throw -- and once the alias key was corrected, it did.
That test is a fifth site of the same typo, and its comment was wrong twice
in one sentence: it called the name "an IFC2X3 leaf that no bundled EXPRESS
export carries" and said ENTITY_NAME_ALIASES "is the only table that knows
it". The name is not an IFC entity at all, so the only thing that knew it was
an alias key misspelled to match; and the real IfcElectricDistributionPoint IS
in ENTITIES_IFC2X3. Verified the corrected name belongs in STANDARD_BUT_UNMAPPED
on its own merits: it is a known type (so ofType does not throw) and has no
TYPE_STRING_TO_ENUM entry (so it resolves to Unknown), which is exactly what
that list is for.
A sixth site: ifc-query.ts's paragraph explaining why the alias table is
consulted used the misspelling as its example of "names real STEP files carry
that the bundled EXPRESS exports omit". It was not an example of that -- the
real entity is in the bundled export -- so the paragraph now says so instead of
illustrating itself with a name that cannot occur.
Not touched: packages/parser/CHANGELOG.md and packages/query/CHANGELOG.md both
carry the misspelling, including a whole released entry titled "Resolve
IfcElectricalDistributionPoint's inheritance chain in the TS parser" (#2883).
Those are published release notes -- a record of what shipped, typo included.
Rewriting history to match a later correction is not the same as fixing the
code, and #2883 is where this thread starts if anyone wants the origin.
Six sites, two languages, and the count kept going up because each fix exposed
the next: the Rust table, a Rust test, the TS alias map, a TS parser test, a TS
query test, and a TS source comment. Every one agreed with the others, and the
suites were green throughout.
cargo test -p ifc-lite-core --lib: 161 passed. The TS half still cannot run
here (no node_modules); CI is the check, which is what found this one.
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
|
Petru, this collided with #3178 and that is on me, not on you. #3178 merged as You identified the same root cause independently: the State on A case-insensitive sweep of Comparing the arm tables directly, main has 26 and this branch has 22, and the only key here that main lacks is That difference is the one real design choice between the two, and it is a preference rather than a gap. You respelled the parser alias as belt and braces; main removed the row instead, on the reasoning that the correctly spelled entity is already in So there is nothing in this branch that main is missing, and rebasing it would mean resolving a conflict in a table both changes rewrote, for no remaining delta. What to do with this PR is your call, or Louis's. I have not touched it. If you would rather the credit sit where the work happened, I am happy to open a follow-up that records your finding, or to say so on #3178. One thing from #3178 that may interest you given how this bug hid: |
|
Superseded — this is already fixed on
And it pins both directions, which is the part I would have argued for: assert!(has_geometry_by_name("IFCELECTRICDISTRIBUTIONPOINT"));
assert!(!has_geometry_by_name("IFCELECTRICALDISTRIBUTIONPOINT"));plus the TS equivalent in This branch is One item that does not appear to exist on |
|
Thanks for tracing it rather than just closing it — and no need to apportion anything. Two people finding the same bug within twenty minutes is a good sign about the sweep, not a problem. I checked the one item I had flagged as possibly missing, and it is not missing — it is mechanized, which is better than what I was offering. My branch carried a unit test asserting the repaired key is absent from the generated schema, i.e. that the legacy table is not shadowing a generated entry. exiting 1, wired into On the alias: I agree it is preference, and on reflection I think I would rather this PR just go, and I have no interest in a follow-up recording the finding — the fix is in and the gate is in, which is the part that mattered. I do not close PRs myself, so it is yours to close whenever convenient. The coverage gate is the useful outcome here. The misspelling survived because nothing could observe it: the row could never match a real file, and the only test pointed at the same wrong string. Failing in both directions is exactly what that needed. |
|
Checked this at head: fail=0, pending=0, but it needs a real rebase, not a mechanical one. Six files conflict, seven markers: These are content conflicts in the legacy-entity tables themselves, on both the Rust and TypeScript sides, so this is not a pin or a digest that can be regenerated. Worth resolving by hand with the schema open. Given what this PR is fixing, one specific caution. A misspelling in this family has already propagated once: the same wrong keyword was mirrored from Rust into TypeScript, reaching five artifacts for an entity that does not exist. So when you resolve, check the two sides agree on the corrected spelling rather than each keeping its own, since a conflict resolution that takes "ours" on one side and "theirs" on the other is exactly how the halves drifted apart in the first place. If there is a test that asserts the Rust and TS tables match, that is the one to run after resolving. I have not touched your branch. Ping me once it is updated and I will review and merge. |
|
This one is conflicted against I have been clearing the allowlist conflicts across tonight's batch, but this PR is a different shape. Seven conflict hunks, and every one has content on both sides: The ones I have been resolving were either the pinned-digest file (mechanical, and I re-derive the numbers from the gate rather than picking) or one-sided hunks where one branch adds and the other does not — those are determined rather than judged. None of these are. Both sides changed the same legacy-entity name handling, and choosing between them is a call about schema-name semantics in your code, not a merge mechanic. Resolving it blind is exactly how a correct-looking merge quietly drops one side's intent. It has also been open since 04:38 and Two things that may help when you pick it up:
Happy to resolve it if you tell me which side wins in |
|
Do not merge this yet. It reads This PR carries 8 checks. A healthy PR on this repo carries 38. The missing lanes: What it does have is Cause: Why no failure count catches this. The required aggregate is not red, it is absent. The fix is one push of any kind — an empty commit, a rebase onto current main, anything creating a new head event now that the base is Worth doing before that push, since the base changed: this has never been merged against current #3185 has the identical shape — 6 checks, same missing set — but it is CONFLICTING so it cannot merge by accident. Same treatment when it is resolved. For anyone sweeping the board: count the checks before reading their status. |
|
Conflicting, and about half of it has landed by another route — worth a look before you spend time rebasing 108 commits. The spelling fix is already on That last one is a negative assertion pinning the misspelling as not matching, which is the right shape and already there. But this PR is not purely superseded. Comparing your head against current So the useful question is whether that remainder is still wanted on its own. If it is, a rebase should shrink this PR a lot — the spelling changes will drop out as already-applied and what is left is the legacy-entity coverage. Merge-base is |
|
Checked the remainder file by file against
The in-crate Rust tests are subsumed by The rest is comments, already rewritten on main — including the So: nothing to rescue, and I would not resolve the conflicts. Leaving it open rather than closing — your call. |
2 participants
Refs #3172 — the typo half of it, which is unambiguous. The larger gap in the same table needs a mapping decision and is untouched here.
IfcElectricDistributionPointis a real, concrete IFC2X3 entity —IfcFlowController, carryingObjectPlacementandRepresentation. Every place that claimed to handle it spelled itIfcElectricalDistributionPoint, with an "AL" that no IFC schema has.Four sites, two languages, all agreeing with each other
rust/core/src/legacy_entities.rsrust/core/src/schema_helpers_tests.rshas_geometry_by_nameon the misspellingpackages/parser/src/ifc-schema.tsENTITY_NAME_ALIASEScarried the same misspelled keypackages/parser/test/known-type-across-schemas.test.tsThe TS alias comment says it mirrors
legacy_entities.rs. It did — typo included.The consequence: the real entity missed the table, fell through to
IfcType::from_str→Unknown, and lost thehas_geometry: truethat arm exists to give it. The meshing pass then skipped it, silently. Nothing failed, because both languages' tests asserted a name that cannot occur, and both passed against tables misspelled the same way.That is why "the tests are green" was not evidence here — the oracle shared the defect.
Verified both directions
Restoring the typo fails
electric_distribution_point_is_spelled_as_the_step_keywordwith "IFCELECTRICDISTRIBUTIONPOINT is a real IFC2X3 keyword and must resolve". The new tests assert the misspelling is absent as well as the real name present, so re-adding it as an extra arm cannot quietly satisfy them.Two things deliberately not done
The TS alias entry is kept, not deleted — though on that side it is redundant.
ENTITIES_IFC2X3does carryIfcElectricDistributionPoint, sogetInheritanceChainreachesIfcDistributionElementthrough its real parents (IfcFlowController→IfcDistributionFlowElement→IfcDistributionElement) with no alias at all. It stays to keep lockstep with the Rust core, whose generated enum is IFC4X3-only and genuinely needs it. The comment there claimed no bundled table carries the entity — false for the corrected name — and is fixed too.base_typestaysIfcDistributionElement. The real parent isIfcFlowController, which exists in the Rust schema and would be more precise, but changing it changes classification for a live entity. That is a separate decision from repairing a name, and yours to make.Verification gap, stated plainly
The TS side is not verified locally: this worktree has no
node_modules, and the one that had them lost its.gitto an unrelated prune mid-session. The TS change is a key rename plus its matching test, and the parent chain quoted above was read out of the bundled table rather than assumed. CI runs those tests.No changeset:
has_geometryfor a previously-unreachable key is a bug fix with no API change, and the packages affected are internal to the parse path — happy to add one if you would rather it appear in the notes.