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

Only accept named nodes as predicates for metadata #466

Closed
joachimvh opened this issue Jan 4, 2021 · 8 comments
Closed

Only accept named nodes as predicates for metadata #466

joachimvh opened this issue Jan 4, 2021 · 8 comments

Comments

@joachimvh
Copy link
Member

Following ae06e99 I think we can remove the toCachedNamedNode function completely. Only the RepresentationMetadata uses it but in practice I think it already receives named nodes most of the time, so we can also change the interface there and always do the call with the .terms uris then.

@RubenVerborgh
Copy link
Member

Check; I was wondering about that when I went through the code indeed. The only exception are the shorthands though, what do we do with contentType?

@joachimvh
Copy link
Member Author

Would have to look at how to specifically do the change, but we can potentially just have an exception for those in the RepresentationMetadata file, but there is no need to have this function publicly available.

@constraintAutomaton
Copy link

constraintAutomaton commented Jun 5, 2021

It seems like every usage of addQuad use a NamedNode as parameters for predicate (which is the only element that called cachedNamedNodes ). Can we restrict the type to only NamedNode.

Every usage of remove uses RDF.type (of type string) which could be replace by a constant of the type NamedNode.

add in LinkMetadataGenerator uses a string. setOveride and getAll use also a string. We could move cachedNamedNodes inside the RepresentationMetadata and make the function private.

@MisterTimn MisterTimn self-assigned this Apr 11, 2022
@MisterTimn
Copy link
Contributor

As a start I went ahead and moved toCachedNamedNode into RepresentationMetadata and getting to know the relevant classes.

Is the desired goal to change all calls to effectively use a NamedNode or is this impossible/cumbersome in some cases? Right now I am going through the remove related function calls I think they can all indeed use it, only in tests are strings still getting used (as far as I can tell).

Any ideas where to best start?

@joachimvh
Copy link
Member Author

As a start I went ahead and moved toCachedNamedNode into RepresentationMetadata and getting to know the relevant classes.

I think the shorthands object can even be removed. I'm pretty sure its contents are never used in practice.

Is the desired goal to change all calls to effectively use a NamedNode or is this impossible/cumbersome in some cases?

The first. This should always be possible. If some classes take a string as input (which will be the case for some constructors probably), it would then be their responsibility to convert the string to a named node.

The one issue/exception is the overrides object in the RepresentationMetadata constructor since the keys have to be strings there. toCachedNamedNode will have to stay for that case.

Any ideas where to best start?

Remove string as an acceptable value for predicates (and subjects) in all RepresentationMetadata functions and see what breaks. My guess is that the number of cases should be limited, except perhaps in the tests.

@MisterTimn
Copy link
Contributor

MisterTimn commented Apr 12, 2022

Remove string as an acceptable value for predicates (and subjects) in all RepresentationMetadata functions and see what breaks. My guess is that the number of cases should be limited, except perhaps in the tests.

I have already done so for remove operations and all seems to be working (after changing some calls with strings to NamedNodes), I'll open a draft PR asap and continue work on this there.

Since I am changing function parameter types, this will be a semver.major probably?

@joachimvh
Copy link
Member Author

Since I am changing function parameter types, this will be a semver.major probably?

Yes, definitely.

@joachimvh
Copy link
Member Author

Done in #1261

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants