Check IfcDateTime according to ISO standards #3864
Conversation
| def year(self, items): | ||
| return ''.join(token.value for token in items) |
There was a problem hiding this comment.
How about something like this:
| def year(self, items): | |
| return ''.join(token.value for token in items) | |
| def _integer(self, items): | |
| return int(''.join(token.value for token in items)) | |
| year = _integer |
and same as below.
I also added the int() I would convert to the appropriate type in the transformer already.
|
|
||
| day: DIGIT DIGIT | ||
| month: DIGIT DIGIT | ||
| year: MINUS? DIGIT+ |
There was a problem hiding this comment.
Wikipedia says:
ISO 8601 prescribes, as a minimum, a four-digit year
Should we add that to the grammar?
| hour: DIGIT DIGIT | ||
| minute: DIGIT DIGIT | ||
| second: DIGIT DIGIT | ||
| millisecond: DIGIT+ |
| print("syntax error") | ||
|
|
||
|
|
||
| if __name__ == '__main__': |
There was a problem hiding this comment.
Please add these as a def test_datetime() in https://github.com/IfcOpenShell/IfcOpenShell/blob/v0.7.0/src/ifcopenshell-python/test/test_validate.py
There was a problem hiding this comment.
Also we should add some test cases of actual IFC file content to check whether the values of such types are correctly discovered. For those you can follow the same convention as here https://github.com/IfcOpenShell/IfcOpenShell/tree/v0.7.0/src/ifcopenshell-python/test/fixtures/validate
E.g create an IfcPropertyListValue with multiple IfcDateTime values. IfcDateTime is IfcSimpleValue is IfcValue.
There was a problem hiding this comment.
test files created d0abe8f
| ) | ||
|
|
||
| except Exception as e: | ||
| #todo: handle aggregations |
There was a problem hiding this comment.
todo..
See here how we have a recursive function to descend into aggregates https://github.com/IfcOpenShell/IfcOpenShell/blob/v0.7.0/src/ifcopenshell-python/ifcopenshell/express/rule_executor.py#L200-L203
|
Fails when we run the tests https://github.com/IfcOpenShell/IfcOpenShell/actions/runs/6607354944/job/17944635714#step:8:37 Probably needs to be a relative import. (relative) Python module import never fails to confuse me. |
|
|
||
| parsing = parse_datetime(val) | ||
|
|
||
| if isinstance(parsing, DateErrorParsing().__class__): |
There was a problem hiding this comment.
| if isinstance(parsing, DateErrorParsing().__class__): | |
| if isinstance(parsing, DateErrorParsing): |
DateErrorParsing().class == DateErrorParsing
| logger.error( | ||
| "For instance:\n %s\n %s\nInvalid attribute value %s for %s.%s", | ||
| inst, | ||
| annotate_inst_attr_pos(inst, i), | ||
| parsing.msg, | ||
| entity, | ||
| attrs[i], | ||
| ) | ||
| elif isinstance(validate_datetime(parsing), DateErrorContent().__class__): | ||
| logger.error( | ||
| "For instance:\n %s\n %s\nInvalid attribute value %s for %s.%s", | ||
| inst, | ||
| annotate_inst_attr_pos(inst, i), | ||
| parsing.msg, | ||
| entity, | ||
| attrs[i], | ||
| ) |
There was a problem hiding this comment.
I think these two branches are equal?
| # unpack the type instance | ||
| yield from check(value[0], value.is_a(), instance=instance) | ||
|
|
||
| if value.is_a() == "IfcDateTime" and value[0]: |
There was a problem hiding this comment.
This function still does the type check here, so can't be reused.
Rather yield the values and do the check outside of the function. That way the same function can also be used for checking uniqueness for example.
| # unpack the type instance | ||
| yield from check(value[0], value.is_a(), instance=instance) | ||
|
|
||
| if value.is_a() == "IfcDateTime" and value[0]: |
There was a problem hiding this comment.
Is it safe to always use value[0]? We need to add some test files where the IfcDateTime is used as an actual attribute such as IfcLibraryInformation and not just a simple type in a select, such as IfcPropertyListValue.
| @@ -365,7 +412,61 @@ def validate(f, logger, express_rules=False): | |||
|
|
|||
| if not has_invalid_value: | |||
| for i, (attr, val, is_derived) in enumerate(zip(attrs, values, entity.derived())): | |||
|
|
|||
| try: | |||
There was a problem hiding this comment.
Why is a try catch statement used here? Catching all subtypes of Exception silences a lot of other issues. Please rewrite to proper if statement.
| ) | ||
| else: | ||
| other_check = list(validate_datetime(r["msg"])) | ||
| if other_check and isinstance(other_check[0], DateErrorContent().__class__): |
| declared_type = type_of_attribute.declared_type() | ||
| type_name = declared_type.name() | ||
|
|
||
| if type_name == "IfcDateTime" and val: |
There was a problem hiding this comment.
I would implement validation of https://ifc43-docs.standards.buildingsmart.org/IFC/RELEASE/IFC4x3/HTML/lexical/IfcDuration.htm in this PR as well. And really make sure that the code is as general and reusable as possible. So that validators are stored for example in a global mapping like:
type_validators = {
'IfcDateTime: [parse_datetime, validate_datetime],
'IfcDuration: [ ... ]
...
}
No description provided.