Skip to content
This repository was archived by the owner on Sep 23, 2025. It is now read-only.

Conversation

@pmcb55
Copy link
Contributor

@pmcb55 pmcb55 commented Feb 3, 2021

Just some comments for discussion - it would be nice to be able to share this file (with explanatory Turtle perhaps) with external parties who may be looking at modelling these ideas using Upper/Mid-Level Ontologies, but it would need some clarifications first I think... (and comments explaining 'missing' terms, etc.)

Just some comments for discussion - it would be nice to be able to share this file (with explanatory Turtle perhaps) with external parties who may be looking at modelling these ideas using Upper/Mid-Level Ontologies, but it would need some clarifications first I think... (and comments explaining 'missing' terms, etc.)
@vercel
Copy link

vercel bot commented Feb 3, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/inrupt/pod-browser/4msw0loah
✅ Preview: https://pod-browser-git-pmcb55-patch-1.inrupt.vercel.app

@pmcb55 pmcb55 marked this pull request as draft February 3, 2021 10:28
Should make it easier to understand each shape and their relationship between each other
@megoth
Copy link
Contributor

megoth commented Feb 4, 2021

I've added some comments that should make each ShEx shape more capable of documenting how they're used

@megoth megoth requested review from ajacksified and timbl February 4, 2021 06:55
@megoth megoth changed the title Update person.shex Update ShEx shapes to be more self-documented Feb 4, 2021
Co-authored-by: Pat McBennett <patm@inrupt.com>
PREFIX xsd: <https://www.w3.org/2001/XMLSchema#>

# This shape describes part of the information we store when adding a group to
# an address book. The resource it is used for is an index of groups. There will
Copy link
Contributor Author

@pmcb55 pmcb55 Feb 5, 2021

Choose a reason for hiding this comment

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

I wouldn't say "The resource it is used for is an index of groups", since it could be reused in a million different ways by other people. You could just add that as an example of where it could be used, but if so I'd suggest rewording it slightly to something like "could be used to represent a group within an index of groups" (but then what's the difference between "an index of groups" and "a collection of groups", or "a container containing groups"...?).

Co-authored-by: Pat McBennett <patm@inrupt.com>
megoth and others added 2 commits February 5, 2021 10:56
Co-authored-by: Pat McBennett <patm@inrupt.com>
Co-authored-by: Pat McBennett <patm@inrupt.com>
Copy link
Contributor

@megoth megoth left a comment

Choose a reason for hiding this comment

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

I think this is a big step in improving the documentation, so I'm happy to approve as-is. But left a couple of suggestions for improvements, but not strictly necessary.

@pmcb55 pmcb55 marked this pull request as ready for review February 10, 2021 14:43
Co-authored-by: Matthieu Bosquet <matthieubosquet@gmail.com>
Co-authored-by: Matthieu Bosquet <matthieubosquet@gmail.com>
Copy link
Contributor

@matthieubosquet matthieubosquet left a comment

Choose a reason for hiding this comment

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

I submitted separate PRs raising concerns about the Address Book entity which I think is essentially a vcard:Group and about the use of a WebID class, which I feel is not appropriate in this context.

I'm also concerned about the idea of caching in index resources. But that would be a separate discussion I believe.

@pmcb55 pmcb55 merged commit 47c0ff4 into main Feb 10, 2021
@pmcb55 pmcb55 deleted the pmcb55-patch-1 branch February 10, 2021 21:50
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants