-
Notifications
You must be signed in to change notification settings - Fork 1
in progress - initial work on Profile for review #7
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
base: main
Are you sure you want to change the base?
Conversation
|
@tgra I'm at a conference, so not necessarily gonna manage to get to this today. A cursory look at your comment indicates that the problem is probably with |
langsamu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would appreciate more consistent whitespace style without superfluous empty lines and other comments from #7.
| /* Roles (inverse org:member) */ | ||
|
|
||
| get roles(): Iterable<Role> { | ||
| return this.objects(ORG.member, Role) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need advice on this -
get roles(): Iterable {
return this.objects(ORG.member, Role)
}
Expected 3 arguments, but got 2.ts(2554)
TermWrapper.d.ts(12, 68): An argument for 'termMapping' was not provided
The method objects indeed requires 3 arguments.
https://github.com/theodi/wrapper/blob/a058232fd9d8076215b9c7dc03dd20e83a88652b/src/TermWrapper.ts#L36-L38
- a
stringthat is the predicate - a
ValueMappingthat converts from aTerminto some native type - a
TermMappingthat converts from some native type into aTerm
Since you want to expose a set of roles, we need to facilitate transformations in both directions (from term to value and from value to term). These are the two arguments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See https://github.com/solid/object/blob/main/src/acp/AccessControl.ts for an example.
|
|
||
| /* Roles (inverse org:member) */ | ||
|
|
||
| get roles(): Iterable<Role> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typing this property as Iterable will make it immutable in TypeScript.
But the this.objects utility returns a Set<T> which is mutable.
Yours is valid modelling, but I imagine that the intent would be to expose the mutable set.
| get roles(): Iterable<Role> { | |
| get roles(): Set<Role> { |
| export class ProfileDataset extends DatasetWrapper { | ||
|
|
||
| get profile(): Iterable<Profile> { | ||
| return this.instancesOf(FOAF.PersonalProfileDocument, Profile) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Picking the profile node (term) out of the graph based on the a foaf:PersonalProfileDocument statement alone is optimistic to say the least.
I strongly recommend looking into how WebID profile graphs are shaped in the various Solid server implementations and other places.
Some related discussion in https://github.com/solid/object/blob/main/src/webid/WebIdDataset.ts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this relate to the existing classes in this folder?
need advice on this -
get roles(): Iterable {
return this.objects(ORG.member, Role)
}
Expected 3 arguments, but got 2.ts(2554)
TermWrapper.d.ts(12, 68): An argument for 'termMapping' was not provided
reference - https://github.com/SolidOS/profile-pane/blob/main/src/ontology/profileForm.ttl