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

Return property defs by type #185

Merged
merged 19 commits into from
May 25, 2020
Merged

Conversation

tobiasschweizer
Copy link
Contributor

@tobiasschweizer tobiasschweizer commented May 14, 2020

@tobiasschweizer tobiasschweizer self-assigned this May 14, 2020
@tobiasschweizer
Copy link
Contributor Author

@flavens You can try it in the UI lib and see if it does what you need

const systemProps = resource.getPropertyDefinitions(SystemPropertyDefinition);
const resProps = resource.getPropertyDefinitions(ResourcePropertyDefinition);

Since the type arg is optional, the return type is generic (PropertyDefinition[]).

We could make the type arg non optional, so the compiler would know whether you asked for system or resource props.

Shall we make another method called getPropertyDefinitionsByType that would then call getPropertyDefinitions (would return all defs) and filter them?

@flavens
Copy link
Contributor

flavens commented May 14, 2020

@flavens You can try it in the UI lib and see if it does what you need

const systemProps = resource.getPropertyDefinitions(SystemPropertyDefinition);
const resProps = resource.getPropertyDefinitions(ResourcePropertyDefinition);

I can get PropertyDefinitions, system + resource. However, I figured out that there are some missing essential information. We need more for each resourceProp:

  • guiDef:this.resource.entityInfo.classes[this.resource.type].propertiesList.[propIndex] -> guiOrder and cardinality
  • values: this.resource.properties[index] -> metadata e.g. arkUrl, attachedToUser, etc.

This is why I created this object propInfoAndValues in resource-view.component.ts, to combine the 3 sources of info:

if (this.resource.entityInfo.properties[index] instanceof ResourcePropertyDefinition) {
              // filter all properties by type ResourcePropertyDefinition
              const propInfoAndValues: PropertyInfoValues = {
                guiDef: prop, <-- missing
                propDef: this.resource.entityInfo.properties[index], <-- given
                values: this.resource.properties[index] <-- missing
              };
              this.propArray.push(propInfoAndValues);
            }

Since the type arg is optional, the return type is generic (PropertyDefinition[]).

We could make the type arg non optional, so the compiler would know whether you asked for system or resource props.

Good idea

Shall we make another method called getPropertyDefinitionsByType that would then call getPropertyDefinitions (would return all defs) and filter them?

Yes that would be convenient!

@tobiasschweizer
Copy link
Contributor Author

@flavens Ok, thanks for your input. If will see how this could fit in.

@tobiasschweizer
Copy link
Contributor Author

@flavens I think now I understood what you mean :-)

You want a collection of cardinalities for a given resource class that

  • can be filtered by type of property def (system or resource property defs)
  • do not only have the the index of the property definition (the Iri), but the actual definition object

I think we should move the new methods created on this branch so far from ReadResource to the OntologyCache because this is also useful even when there is no instance of a resource, e.g. for the search.

https://github.com/dasch-swiss/knora-api-js-lib/blob/ecb715718ea91e7fdad6beca704b8974e4b07f75/src/cache/OntologyCache.ts#L11-L26

I think this has some consequences on the current implementation of IResourceClassAndPropertyDefinitions :

  • IResourceClassAndPropertyDefinitions needs to be a class, not an interface so it can have methods
  • these additional methods have to be added to IResourceClassAndPropertyDefinitions

Maybe this can be done in a backward compatible way.

I now remember the reason for having propertyIndex in the cardinalities: it was just simpler to create the structure when deserializing JSON-LD, that's it. Maybe this just needs a second step: Once all the information is there, we can build something like the propInfoAndValues.

I suggest that we continue on this on Monday. Would that be ok?

@flavens
Copy link
Contributor

flavens commented May 15, 2020

I suggest that we continue on this on Monday. Would that be ok?

Yes it's ok! I will then take a look at IResourceClassAndPropertyDefinitions and the other involved files to improve my knowledge of dsp-js.

@tobiasschweizer
Copy link
Contributor Author

@flavens Now the property definitions are contained in the cardinalities (not just the property index). Would this be enough for your use-case?

The implementation is not good however. I need to figure out how I could transform in between data structures ResourceClassDefinition and ResourceClassDefinitionWithPropertyDefinition in a more elegant way. Also the ontology mock has to be kept in snyc.

@flavens
Copy link
Contributor

flavens commented May 19, 2020

@tobiasschweizer which is the new method I can use in dsp-ui to get the properties with all information? Your previous setup does not work anymore.
const systemProps = this.resource.getPropertyDefinitions(SystemPropertyDefinition);
const resProps = this.resource.getPropertyDefinitions(ResourcePropertyDefinition);

@tobiasschweizer
Copy link
Contributor Author

@tobiasschweizer which is the new method I can use in dsp-ui to get the properties with all information? Your previous setup does not work anymore.
const systemProps = this.resource.getPropertyDefinitions(SystemPropertyDefinition);
const resProps = this.resource.getPropertyDefinitions(ResourcePropertyDefinition);

I will have to move some methods. Could you just check entityInfo on ReadResource? In a cardinality, you should now also get the property def, not just the index.

'{"propertyIndex":"http://api.knora.org/ontology/knora-api/v2#hasStandoffLinkToValue","cardinality":2,"isInherited":true,"propertyDefinition":{"id":"http://api.knora.org/ontology/knora-api/v2#hasStandoffLinkToValue","subPropertyOf":["http://api.knora.org/ontology/knora-api/v2#hasLinkToValue"],"comment":"Represents a link in standoff markup from one resource to another.","label":"has Standoff Link to","subjectType":"http://api.knora.org/ontology/knora-api/v2#Resource","objectType":"http://api.knora.org/ontology/knora-api/v2#LinkValue","isLinkProperty":false,"isLinkValueProperty":true,"isEditable":false,"guiAttributes":[]}}'

@tobiasschweizer
Copy link
Contributor Author

@flavens In the test app, I added the logic you had in the UI lib: c7d3023

This is just for now and will be removed later from the test app.

@tobiasschweizer tobiasschweizer added enhancement Improve existing code or new feature feature labels May 25, 2020
Copy link
Contributor

@flavens flavens left a comment

Choose a reason for hiding this comment

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

LGTM

@tobiasschweizer
Copy link
Contributor Author

tobiasschweizer commented May 25, 2020

@lrosenth This PR changes the way the entity info is made available on a ReadResource. However, this change is backward compatible.

entityInfo: IResourceClassAndPropertyDefinitions;

changed to

entityInfo: ResourceClassAndPropertyDefinitions;

IResourceClassAndPropertyDefinitions has been changed from an interface to a class.

The resource class will now be represented by a ResourceClassDefinitionWithPropertyDefinition that is derived from ResourceClassDefinition which was originally returned. ResourceClassDefinitionWithPropertyDefinition contains the property definitions the resource class has cardinalities for, ResourceClassDefinition just contains the properties' index.

In short, more information is returned now and some methods have been added, but existing code won't break (only the name change could be an issue).

@tobiasschweizer tobiasschweizer merged commit a28e64b into master May 25, 2020
@tobiasschweizer tobiasschweizer deleted the wip/DSP-142-property-defs branch May 25, 2020 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improve existing code or new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants