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

ALS003_Alignment-cant-shape-representation #63

Conversation

JTurbakiewicz
Copy link
Contributor

No description provided.

@JTurbakiewicz JTurbakiewicz requested review from pjanck, Ghesselink, aothms and evandroAlfieri and removed request for pjanck and Ghesselink April 23, 2023 23:13
features/RI014_Alignment-cant-shape-representation.feature Outdated Show resolved Hide resolved
features/steps/thens/attributes.py Outdated Show resolved Hide resolved
Comment on lines 88 to 89
if attribute_value != value:
errors.append(err.InvalidValueError(instance, attribute, attribute_value))
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe I have already asked this somewhere, but for the sake of completeness: Does this check account for different types? Or does it only compare string literals?

Copy link
Contributor Author

@JTurbakiewicz JTurbakiewicz Apr 26, 2023

Choose a reason for hiding this comment

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

It only compares to string literals.

@then('The value of attribute {attribute} must be {value}')

{value} is cast to a string by behave parser, so all comparisons are made to a string. For the purposes of the rules currently developed that's ok. For general purposes, it's more complex, but I think it's very difficult to introduce type checks without assuming the .feature file writer/reader is familiar with types.

As we once discussed, the .feature file user would have to be able to distinguish between:

And The value of attribute XYZ must be 1
And The value of attribute XYZ must be 1.0
And The value of attribute XYZ must be "1"

or even going to extremes (but I'm not quite sure if the below will ever occur):

And The value of attribute XYZ must be "Class"
And The value of attribute XYZ must be Class (as if Python class).

Explicit declaration of type requires even more from the user, so for example:

And The value of attribute XYZ must be 1 of integer type

@evandroAlfieri @aothms @Ghesselink tagging as it's a tough question.

Copy link
Contributor

@Ghesselink Ghesselink May 5, 2023

Choose a reason for hiding this comment

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

For me it seems that comparing it to string literals should be fine.

As we once discussed, the .feature file user would have to be able to distinguish

Is there an instance in a IFC file where this distinction, string '3' as opposed to integer 3 as value, is present and in which usecases would this matter? Maybe for a software vendor who is very specific regarding implementing this information in a drawing, or something like that.

I think in general it is better to keep the requirements from the user in the feature file as simple as possible. But if this usecase comes up we could solve it with an extra decorator. In this example:
And The value of attribute XYZ must be 1 of integer type
The decorators would be something like

@{The value of attribute {attribute} must be {value}
@{The value of attribute {attribute} must be {value}{optional_data_type}
def step_impl(attribute, value, optional_data_type = 'string')
[...] 
spl = optional_data_type.split()
if any(value in spl for value in ['integer', 'int']: 
  type = int
elif any(value in spl for value in [something else, x, y]: #in case of more destinctions: Python classes, ifc_openshell_instances, colours of the rainbow, etc.. 
    type = ifcopenshell.entity_instance #for example
else: 
  type = str #default should be string
def process_string(data):
    return type(data)
if process_string(attribute_value) != process_string(value) #for the mentioned example
   errors.append(err.InvalidValueError[.....])

Didn't test it thoroughly, but I could imagine some use-cases for this.

@evandroAlfieri
Copy link
Collaborator

This is still under discussion in the IF, but still a very good and reusable rule (also for horizontal and vertical, as soon as we get to a conclusion). Meanwhile, please change all RI-14 with ALS003

@JTurbakiewicz JTurbakiewicz changed the title RI014_Alignment-cant-shape-representation ALS003_Alignment-cant-shape-representation Apr 26, 2023
@aothms aothms merged commit 7341440 into buildingSMART:main Sep 18, 2023
2 checks passed
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.

5 participants