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

[AID] Handling of semantic annotations #54

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

egekorkan
Copy link
Member

If a TD has a semantic annotation, it is simply ignored in the conversion. This PR adds a basic support in this direction. I am not exactly satisfied since there can be many corner cases. The worst is if there is an additional context without a prefix. That would mean looking into the context for each possible unknown key. I have added a good amount of comments in the code but also adding now some in the PR review.

node/aas-aid/tsconfig.json Show resolved Hide resolved

if (Array.isArray(tdContext)) {
if (tdContext.length > 3) {
// if it is longer than 3, assume 3rd item is the object. This offset is needed as node-wot always adds @language at the end
Copy link
Member Author

Choose a reason for hiding this comment

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

This offset can be avoided if we do not normalize the TD when parsing. In any case, I find it a bit weird that node-wot changes the context.

Also this really assumes that we have the TD contexts (v1 or v1.1) and then an object with prefixes.

Copy link
Member

Choose a reason for hiding this comment

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

Mhh, not sure what is meant by "node-wot changes the context". Should we open an issue on node-wot side?
Anyhow, it should work in any order ... but I do recall that in the TD spec there is a order defined... maybe that is the reason?

image

Copy link
Member

Choose a reason for hiding this comment

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

In general I think this part of the code assumes some characteristics that might change over time in node-wot. Not sure if this future proof...

Copy link
Member Author

Choose a reason for hiding this comment

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

I also feel that this is not future-proof but I have thought of a small improvement: Going through all object items in the array and merging them into one object.

node-wot is not doing anything against the TD spec but it always adds an object with @language in it. So the TD I provide is changed after the parsing.

@@ -1213,12 +1213,15 @@ export class AssetInterfacesDescription {
for (const propertyKey in td.properties) {
const property: PropertyElement = td.properties[propertyKey];

// we get all the property keys and remove known keys by the TD spec at each if clause
let propertyKeys: any = Object.keys(property);
Copy link
Member Author

Choose a reason for hiding this comment

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

This propertyKeys handling is the part I find a big ugly but I could not find a better solution. I went initially for a full json-ld processing but it me messed up... So each time the conversion finds a known TD term, it gets removed from the array. In the end, from the unknown terms, the ones with with a : are taken for adding their semantic id with some basic json-ld processing

Copy link
Member

@danielpeintner danielpeintner left a comment

Choose a reason for hiding this comment

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

Find some comments below.

What I would highly recommend is to add some test-cases by either updating some samples or adding new ones.

EDIT: Some CI tests are failing which proves we should add more tests.

node/aas-aid/src/asset-interfaces-description.ts Outdated Show resolved Hide resolved

if (Array.isArray(tdContext)) {
if (tdContext.length > 3) {
// if it is longer than 3, assume 3rd item is the object. This offset is needed as node-wot always adds @language at the end
Copy link
Member

Choose a reason for hiding this comment

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

In general I think this part of the code assumes some characteristics that might change over time in node-wot. Not sure if this future proof...

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.

2 participants