Skip to content
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

Val445 pse001 refinements 2 #191

Merged
merged 9 commits into from
May 13, 2024
Merged

Conversation

aothms
Copy link
Collaborator

@aothms aothms commented May 9, 2024

No description provided.

@aothms aothms requested a review from Ghesselink May 9, 2024 14:05
Copy link
Contributor

@Ghesselink Ghesselink left a comment

Choose a reason for hiding this comment

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

Nice addition :) I tried to read again a bit more into psets and have a couple of comments, but don't think they'd all be part of this PR if addressed.

features/steps/thens/relations.py Outdated Show resolved Hide resolved
features/steps/thens/relations.py Outdated Show resolved Hide resolved
features/steps/thens/relations.py Outdated Show resolved Hide resolved
features/steps/thens/relations.py Outdated Show resolved Hide resolved
features/steps/thens/relations.py Outdated Show resolved Hide resolved
accepted_values['predefined_types'] = sorted(set(s.upper() for s in accepted_values['predefined_types']))

# @todo this is actually wrong, we should consider pairs of <entity, predefined type> together
# and not separately. But in the majority of cases this will be good enough.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm thinking of a way to handle this, e.g.

Scenario Outline:
Given an <IfcEntity>
Given its PropertySet
Then the value must be one of <X>
#or The value must be in <Y>

Examples:
Entity | X
EntityA | Value1,Value2,Value3
EntityB | Value4, Value5, Value6
EntityC | Value7, Value8, Value9

#or 
Entity A | AllowedPredefinedTypesX
Entity A | AllowedPredefinedTypesY

Perhaps at least for the ones for which errors are most likely to occur (to our knowledge) and exclude them from .

Or we can do it the other way around and be more restrictive at

  if 'IfcPropertySet Name attribute value must use predefined values according' in context.step.name:
       if name not in property_set_definitions.keys():
           yield ValidationOutcome(inst=inst, observed = {'value':name}, severity=OutcomeSeverity.ERROR)

and defined specific psets for values (in a separate csv) for the inverse of inst (which is a pset)?
context.model.get_inverse(inst)

So for IfcWall, that would be one of the following values;
https://ifc43-docs.standards.buildingsmart.org/IFC/RELEASE/IFC4x3/HTML/lexical/IfcWall.htm#6.1.3.41.5-Property-sets

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I do like the idea to offload more of the python code to gherkin to make it more explicit. Especially the predefined type handling is now fully opaque in gherkin. It would be nice if we have a specific statement for that. Also filtering on whether the predefined type is required and/or present.

Copy link
Contributor

Choose a reason for hiding this comment

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

Handled in #192

aothms and others added 6 commits May 11, 2024 11:25
Co-authored-by: Geert Hesselink <54070862+Ghesselink@users.noreply.github.com>
Co-authored-by: Geert Hesselink <54070862+Ghesselink@users.noreply.github.com>
Co-authored-by: Geert Hesselink <54070862+Ghesselink@users.noreply.github.com>
Co-authored-by: Geert Hesselink <54070862+Ghesselink@users.noreply.github.com>
@aothms aothms requested a review from Ghesselink May 12, 2024 09:13
@aothms aothms merged commit dbd1673 into development May 13, 2024
2 checks passed
@aothms aothms deleted the val445-pse001-refinements-2 branch May 13, 2024 06:25
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.

None yet

2 participants