-
Notifications
You must be signed in to change notification settings - Fork 18
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
ALS004_Alignment-segment-shape-representation #62
ALS004_Alignment-segment-shape-representation #62
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work :)
Should we formulate the rule (and the one of ALB002 I wrote before as well) slightly different to allow for more reuse of the already written code, e.i. 'The value must be X'
ifc-gherkin-rules/features/steps/thens/values.py
Lines 11 to 66 in 47b6a14
@then("The value must {constraint}") | |
@then("The values must {constraint}") | |
@then('At least "{num:d}" value must {constraint}') | |
@then('At least "{num:d}" values must {constraint}') | |
def step_impl(context, constraint, num=None): | |
errors = [] | |
within_model = getattr(context, 'within_model', False) | |
#to account for order-dependency of removing characters from constraint | |
while constraint.startswith('be ') or constraint.startswith('in '): | |
constraint = constraint[3:] | |
if getattr(context, 'applicable', True): | |
stack_tree = list( | |
filter(None, list(map(lambda layer: layer.get('instances'), context._stack)))) | |
instances = [context.instances] if within_model else context.instances | |
if constraint in ('identical', 'unique'): | |
for i, values in enumerate(instances): | |
if not values: | |
continue | |
attribute = getattr(context, 'attribute', None) | |
if constraint == 'identical' and not all([values[0] == i for i in values]): | |
incorrect_values = values # a more general approach of going through stack frames to return relevant information in error message? | |
incorrect_insts = stack_tree[-1] | |
errors.append(err.IdenticalValuesError(incorrect_insts, incorrect_values, attribute,)) | |
if constraint == 'unique': | |
seen = set() | |
duplicates = [x for x in values if x in seen or seen.add(x)] | |
if not duplicates: | |
continue | |
inst_tree = [t[i] for t in stack_tree] | |
inst = inst_tree[-1] | |
incorrect_insts = [inst_tree[1][i] for i, x in enumerate(values) if x in duplicates] | |
incorrect_values = duplicates | |
# avoid mentioning ifcopenshell.entity_instance twice in error message | |
report_incorrect_insts = any(misc.map_state(values, lambda v: misc.do_try( | |
lambda: isinstance(v, ifcopenshell.entity_instance), False))) | |
errors.append(err.DuplicateValueError(inst, incorrect_values, attribute, incorrect_insts, report_incorrect_insts)) | |
if constraint[-5:] == ".csv'": | |
csv_name = constraint.strip("'") | |
for i, values in enumerate(instances): | |
if not values: | |
continue | |
attribute = getattr(context, 'attribute', None) | |
dirname = os.path.dirname(__file__) | |
filename = Path(dirname).parent.parent / "resources" / csv_name | |
valid_values = [row[0] for row in csv.reader(open(filename))] | |
for iv, value in enumerate(values): | |
if not value in valid_values: | |
errors.append(err.InvalidValueError([t[i] for t in stack_tree][1][iv], attribute, value)) | |
misc.handle_errors(context, errors) |
An idea would then be (something like) :
Scenario 1 : Agreement on Representation being Axis
Given a file with Schema Identifier "IFC4X3_TC1" or "IFC4X3_ADD1" or "IFC4X3"
And An IfcAlignmentSegment
And Its attribute Representation
And Its attribute Representations
And Its attribute RepresentationIdentifier
Then Its value must be 'Axis'
Scenario 2 : Agreement on RepresentationType being Segment
Given Repeat steps 1,2,3,4,5 #maybe change this to 1-5?
Then The value must be Segment
The functionality of this 'Repeat steps' can be found in the open PR for GEM005
ifc-gherkin-rules/features/steps/giventhens/repeat_return.py
Lines 6 to 17 in 0f2a256
@given("Repeat step {step_count}") | |
@given("Repeat steps {step_count}") | |
@then("Repeat step {step_count}") | |
@then("Repeat steps {step_count}") | |
def step_impl(context, step_count): | |
step_stack = list( | |
filter(None, list(map(lambda layer: layer.get('step'), context._stack)))) | |
step_stack.reverse() | |
step_count = list(step_count.split(',')) | |
steps = ('\n').join( | |
[(' ').join(['Given', step_stack[int(n)-2].name + ' ']) for n in step_count]) | |
context.execute_steps(steps) |
Scenario 3 : Agreement on items being IfcCurveSegment
Given repeat steps 1 - 5
Then The value must be 'IfcCurveSegment' #or change to 'items' and add @ to the step impl
This way we get rid of some duplicate code and reuse 'The value must be {constraint}'
often. We could even distinquish between 'type' and 'value' in the gherkin implementation by adding 'The {type/value/items} must be {constraint}
and perhaps the feature file itself becomes more readable (we could potentially use the scenario description as a description of what is being tested for the platform itself).
But maybe I am making it too complicated, I'm not sure .. What do you think?
test/files/ri009/pass-ri009-alignment-segment-shape-representation.ifc
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Just the unfortunate renaming of all RI009.
test/files/ri009/fail-ri009-scenario01-wrong-representationidentifier-value.ifc
Outdated
Show resolved
Hide resolved
test/files/ri009/fail-ri009-scenario01-wrong-representationtype-value.ifc
Outdated
Show resolved
Hide resolved
test/files/ri009/pass-ri009-alignment-segment-shape-representation.ifc
Outdated
Show resolved
Hide resolved
Hi! Thank you for a great comment. I think to setup multiple scenarios with repeatable code we could also use the Background feature, as experimented here: https://github.com/buildingSMART/ifc-gherkin-rules/pull/37/files#diff-f12e75ce63549024543ab0c71a93b3cc880178205d26599482a386aa6041d205R5 I find it a bit more readable than As for using the already present step: 'The value must be {constraint}' (thens/values.py).
And I would add the logic checking the value being equal to the provided value to this step, as currently {constraint} checks for 'identical', 'unique' mostly. But I do agree that it would perhaps be cleaner to use this step. What do you think? @Ghesselink |
I agree; the way I understand the proposal, I also think that this reinvents an existing Gherkin feature: the Background keyword.
This is a code-architecture decision. I don't have any strong opinions on this. |
Agree with both of you @pjanck @JTurbakiewicz . In this case it seems clearer. So then it would be something like :
This would look very clear indeed, nice :-)
That's not required I think as it's already covered by the
While I am writing this down, I think I'm leaning towards the first option. It seems to me that it will more understandable for someone else reading or making own gherkin statements (including future us).
I'll test and work this out further, but would first like to know if you agree/I make sense :) @JTurbakiewicz |
Makes sense. I'll play with it a little bit as well. The additional constraint of ours is to have all the scenarios, so:
Be present in the same time. I'll check if background scenario lets us do that. |
No description provided.