buildingSMART / ifc-validation-data-model

MIT License
5 stars 1 forks source link

Requests from TK #2

Open aothms opened 9 months ago

aothms commented 9 months ago

The following request/questions came from a quick interaction with the datamodel.

1. IfcValidationOutcome.instance points to model instead of instance

https://github.com/buildingSMART/ifc-validation-data-model/blob/d09fb3a4eefd8c96b88fd9655b05698aa88f6359/models.py#L705-L706

2. IfcValidationOutcome.instance should be optional. In not all cases we will have an instance to refer to

3. IfcValidationOutcome.feature and code are redundant. Delete code.

https://github.com/buildingSMART/ifc-validation-data-model/blob/d09fb3a4eefd8c96b88fd9655b05698aa88f6359/models.py#L725-L730

https://github.com/buildingSMART/ifc-validation-data-model/blob/d09fb3a4eefd8c96b88fd9655b05698aa88f6359/models.py#L739-L745

4. IfcValidationOutcome.feature and feature_version Should be optional as not everything is Gherkin-related.

5. My preference would be to remove the Ifc- prefix from all classes

6. IfcModel has no attribute type, suggest file_name. I didn't review all repr functions, just came across this in my repl session.

https://github.com/buildingSMART/ifc-validation-data-model/blob/d09fb3a4eefd8c96b88fd9655b05698aa88f6359/models.py#L381

7. IfcModel.Status: Needed? Or retrieve as related IfcValidationTask.Status? We have it currently in the datamodel, but in my mind it is a bit redundant.

https://github.com/buildingSMART/ifc-validation-data-model/blob/d09fb3a4eefd8c96b88fd9655b05698aa88f6359/models.py#L185-L191

https://github.com/buildingSMART/ifc-validation-data-model/blob/d09fb3a4eefd8c96b88fd9655b05698aa88f6359/models.py#L280-L288

etc.

vs.

https://github.com/buildingSMART/ifc-validation-data-model/blob/d09fb3a4eefd8c96b88fd9655b05698aa88f6359/models.py#L554-L563

8. IfcValidationTask vs IfcValidationRequest. Why both?

https://github.com/buildingSMART/ifc-validation-data-model/blob/d09fb3a4eefd8c96b88fd9655b05698aa88f6359/models.py#L434

https://github.com/buildingSMART/ifc-validation-data-model/blob/d09fb3a4eefd8c96b88fd9655b05698aa88f6359/models.py#L535

9. IfcValidationTask/IfcValidationRequest. No foreign key to IfcModel

rw-bsi commented 9 months ago

Fixes

1. IfcValidationOutcome.instance points to model instead of instance

FIXED

2. IfcValidationOutcome.instance should be optional. In not all cases we will have an instance to refer to

UPDATED

3. IfcValidationOutcome.feature and code are redundant. Delete code.

UPDATED

~~ 4. IfcValidationOutcome.feature and feature_version Should be optional as not everything is Gherkin-related.~~

UPDATED

5. My preference would be to remove the Ifc- prefix from all classes

TBC - I see pro/con, mainly driven by what I saw in ifc standard when I was getting familiar with the domain, but can see that it will be confusing and will potentially lead to name clashes. Will replace it, yet in the table names I still prefer a ifc_ prefix

https://github.com/buildingSMART/ifc-validation-data-model/blob/29cd2179bf69af3dcbf3fbc4b9ae36046d2b13af/models.py#L374-L377

image

6. IfcModel has no attribute type, suggest file_name. I didn't review all repr functions, just came across this in my repl session.

UPDATED (for IfcModel; will update repr where you and I see fit; mainly used in Django Admin for now)

7. IfcModel.Status: Needed? Or retrieve as related IfcValidationTask.Status? We have it currently in the datamodel, but in my mind it is a bit redundant.

Added it initially for performance/backwards compatibility reasons - can be removed

8. IfcValidationTask vs IfcValidationRequest. Why both?

one request leads to multiple sub- tasks

image

9. IfcValidationTask/IfcValidationRequest. No foreign key to IfcModel

UPDATED: added request.model; Tasks can navigate via task.request.model

aothms commented 9 months ago

TBC - I see pro/con, mainly driven by what I saw in ifc standard when I was getting familiar with the domain, but can see that it will be confusing and will potentially lead to name clashes. Will replace it, yet in the table names I still prefer a ifc_ prefix

That's ok. I think it's especially the combination of IfcPascalCase style with IfcPrefix that may lead to confusion as to whether we're talking about concepts from the IFC schema or from our own application. E.g IfcApplication is from the schema, we've called it an IfcAuthoringTool, but it could have easily collided.

one request leads to multiple sub- tasks

Ok that's what I thought and I think that's good, so we can rerun requests on models. But I think the current fields on Request make it a bit confusing as it duplicates fields from both model and task.

Model
  |
  |
  | 1..*
  v
Request
  | 
  | 
  | 7
  v 
Task

May I suggest:

Later we can: