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

Schema.regen.network browser & RDFS Ontology POC #54

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

Conversation

clevinson
Copy link
Member

Description

This PR sets up a basic ontology file in rdfs/regen.ttl and a corresponding next.js application that can be used to render and explore an RDFS ontology for RDF datasets and standards used within Regen Network.

To run locally:

cd schema-www
yarn install
yarn dev

Warning: Most of this code was written in collaboration with chatGPT, this is my first time coding a next.js app, or using tailwind. Please forgive me if there are antipatterns in this PR, I'll happily research and fix anything that folks point out as a problem or something sub-optimal with the way this code is currently laid out.


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • provided a link to the relevant issue or specification
  • reviewed "Files changed" and left comments if necessary

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items
.

I have...

  • confirmed all author checklist items have been addressed
  • reviewed code correctness and readability
  • reviewed documentation is accurate
  • manually tested (if applicable)

@@ -0,0 +1,84 @@
@prefix rdf: <http://www.w3.org/1999/02/22-rdf-syntax-ns#> .
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 is mostly a dummy file with some basic data to see what setting up a website that renders based on RDFS ontologies would look like.

After further research, I do think that it might be best for us to lean into OWL ontologies instead, but curious to hear others' feedback on that.

Copy link
Member

Choose a reason for hiding this comment

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

could you share more on that @clevinson?
BTW reading the articles you've shared is on my TODO list https://archive.topquadrant.com/owl-blog/ vs https://triply.cc/blog/2021-08-why-we-use-owl/

Copy link

@flagrede flagrede left a comment

Choose a reason for hiding this comment

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

Nice work! Left some comments on Tailwind, NextJS, and some React best practices.

schema-www/src/styles.module.css Outdated Show resolved Hide resolved
schema-www/src/pages/_app.tsx Outdated Show resolved Hide resolved
schema-www/src/components/ClassView.tsx Outdated Show resolved Hide resolved
schema-www/tailwind.config.js Outdated Show resolved Hide resolved
schema-www/src/utils/helpers.tsx Outdated Show resolved Hide resolved
Comment on lines 127 to 141
if (item.type === "class") {
return <ClassView {...item} />;
} else if (item.type === "property") {
return <PropertyView {...item} />;
} else {
return (
<div className={styles.error}>
<h1 className={styles.title}>Error: Invalid schema type</h1>
<p className={styles.text}>
The provided schema type is not recognized. Please ensure it is either
&quot;class&quot; or &quot;property&quot;.
</p>
</div>
);
}

Choose a reason for hiding this comment

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

It's more like a convention but conditionally render item is often in JSX directly to improve readability:

Suggested change
if (item.type === "class") {
return <ClassView {...item} />;
} else if (item.type === "property") {
return <PropertyView {...item} />;
} else {
return (
<div className={styles.error}>
<h1 className={styles.title}>Error: Invalid schema type</h1>
<p className={styles.text}>
The provided schema type is not recognized. Please ensure it is either
&quot;class&quot; or &quot;property&quot;.
</p>
</div>
);
}
const isClass = item.type === "class";
const isProperty = item.type === "property";
const isDefault = !isClass & !isProperty
return (
<>
{isClass && <ClassView {...item} />;}
{isItem && <PropertyView {...item} />;}
{isDefault && <div className={styles.error}>
<h1 className={styles.title}>Error: Invalid schema type</h1>
<p className={styles.text}>
The provided schema type is not recognized. Please ensure it is either
&quot;class&quot; or &quot;property&quot;.
</p>
</div>}
</>
);
}

Choose a reason for hiding this comment

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

I actually don't prefer the approach @flagrede outlines here just because it isn't clearly conditional what renders (IE at a glance, it's not clear if two of those conditions can be met at the same time), but it's a bit more clear if you return early on the conditional cases and avoid else blocks:

  if (item.type === "class") {
    return <ClassView {...item} />;
  } 
  if (item.type === "property") {
    return <PropertyView {...item} />;
  }
  return (
    <div className={styles.error}>
      <h1 className={styles.title}>Error: Invalid schema type</h1>
      <p className={styles.text}>
        The provided schema type is not recognized. Please ensure it is either
        &quot;class&quot; or &quot;property&quot;.
      </p>
    </div>
  );
}

Copy link

@flagrede flagrede May 31, 2023

Choose a reason for hiding this comment

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

Interesting, I didn't know we had different thoughts on this topic within the team. Regarding the point you outline @haveanicedavid if a component is meant to render only one of its children at a time then using a state that is unique (like state === 'class') instead of variables for each block clears the confusion. That way you have the same clarity while having only a single return for the component.

Copy link
Member Author

Choose a reason for hiding this comment

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

Is this what you meant @flagrede ? 7a65b03

Choose a reason for hiding this comment

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

Not exactly, this is what I meant: https://github.com/regen-network/regen-registry-standards/pull/54/files#r1226226721
Whenever possible I try to have only one return statement by component. Here since the condition to return is based on a unique state you can be sure that you're never going to have isClass and isProperty at the same time. In case your condition is more complex (it depends on several variables) you can still use that pattern by having a util function that returns a unique state (ie: const myState = getComponent(item.type, item.value, item.name). Does that make sense?

Choose a reason for hiding this comment

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

Whenever possible I try to have only one return statement by component
I actually prefer multiple return statements vs having conditionals live within one JSX block because I find it easier to read / understand. Not trying to argue with you here btw, I think the suggested changes you made are perfectly fine, just wanted to explain my reasoning:

If I see several && statements within one JSX block, I have to check whatever those conditions are and mentally grok whether or not they could happen at the same time, conflict with each other, etc. If I see multiple returns statements, I can read them sequentially knowing that if the first condition is met, the rest won't render -- to me, it's just a bit simpler.

Like many of my code pattern opinions, it isn't really something i'm passionate about, just a preference. As long as things are easy to read (which everything you offered in feedback are!), I don't mind which approach is taken

tldr: the suggestions @flagrede is making sound good to me (even if I prefer something a bit different)

schema-www/src/styles.module.css Outdated Show resolved Hide resolved
schema-www/src/components/ClassView.tsx Outdated Show resolved Hide resolved
Comment on lines 127 to 141
if (item.type === "class") {
return <ClassView {...item} />;
} else if (item.type === "property") {
return <PropertyView {...item} />;
} else {
return (
<div className={styles.error}>
<h1 className={styles.title}>Error: Invalid schema type</h1>
<p className={styles.text}>
The provided schema type is not recognized. Please ensure it is either
&quot;class&quot; or &quot;property&quot;.
</p>
</div>
);
}

Choose a reason for hiding this comment

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

I actually don't prefer the approach @flagrede outlines here just because it isn't clearly conditional what renders (IE at a glance, it's not clear if two of those conditions can be met at the same time), but it's a bit more clear if you return early on the conditional cases and avoid else blocks:

  if (item.type === "class") {
    return <ClassView {...item} />;
  } 
  if (item.type === "property") {
    return <PropertyView {...item} />;
  }
  return (
    <div className={styles.error}>
      <h1 className={styles.title}>Error: Invalid schema type</h1>
      <p className={styles.text}>
        The provided schema type is not recognized. Please ensure it is either
        &quot;class&quot; or &quot;property&quot;.
      </p>
    </div>
  );
}

Comment on lines +1 to +15
export interface Property {
type: "property";
iri: string;
label: string;
description: string;
ranges: string[];
domains: string[];
}
export interface Class {
type: "class";
iri: string;
label: string;
description: string;
properties: Property[];
}

Choose a reason for hiding this comment

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

Probably best to avoid a global types index file like this for individual component prop definitions. Works well for simple stuff, but doesn't scale

Copy link
Member Author

Choose a reason for hiding this comment

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

What would you suggest instead? two type files, Property.ts and Class.ts each with one interface in a file?

Choose a reason for hiding this comment

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

sorry for the late reply - both of these seem to correspond to a react component, and I'd just define the props directly in those components (define the Class interface in ClassView.tsx etc)

@clevinson clevinson marked this pull request as ready for review June 11, 2023 01:50
Copy link
Member

@blushi blushi left a comment

Choose a reason for hiding this comment

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

great to see this finally being worked on!

@@ -0,0 +1,84 @@
@prefix rdf: <http://www.w3.org/1999/02/22-rdf-syntax-ns#> .
Copy link
Member

Choose a reason for hiding this comment

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

could you share more on that @clevinson?
BTW reading the articles you've shared is on my TODO list https://archive.topquadrant.com/owl-blog/ vs https://triply.cc/blog/2021-08-why-we-use-owl/

@aaronc
Copy link
Member

aaronc commented Dec 5, 2023

The biggest issue here I think is the usage of RDFS rather than reusing our existing SHACL files. Regardless of the benefits of RDFS vs OWL vs SHACL for this use case, I think we should be starting with the assumption that we're merging all the .ttl files in this repo and then creating a browser for them. If we think we need RDFS or OWL, we should add the relevant RDFS/OWL triples to the relevant .ttl files themselves in addition to the SHACL that's already there. It would be good to hear more of your thoughts on this, however, @clevinson and what your reasoning is for taking this approach.

@clevinson
Copy link
Member Author

This is the slack thread where we were having most discussion around this: https://regen-network.slack.com/archives/C01LX9E8QN8/p1683743649349819

As I understood it, SHACL is a constraints language designed specifically for validating JSON-LD documents, which is similar, but different, from defining vocabularies.

The main example I used in my head was that if we had a property regen:ecosystemType with values a finite set of potential values, there isn't really a clear way to define this in SHACL without bounding the property to a specific object (e.g. a regen:Project or regen:CreditClass). And that we would actually need to define the constraints of ecosystem type on both the regen:Project and regen:CreditClass definitions.

Using something like OWL or RDFS (I think actually only OWL has the ability to express finite set of values for a property), one could actually define the property regen:ecosystemType and then separately say which classes can have this property.

Let me know if you think I'm misunderstanding something here @aaronc

@aaronc
Copy link
Member

aaronc commented Dec 5, 2023

This is the slack thread where we were having most discussion around this: https://regen-network.slack.com/archives/C01LX9E8QN8/p1683743649349819

As I understood it, SHACL is a constraints language designed specifically for validating JSON-LD documents, which is similar, but different, from defining vocabularies.

The main example I used in my head was that if we had a property regen:ecosystemType with values a finite set of potential values, there isn't really a clear way to define this in SHACL without bounding the property to a specific object (e.g. a regen:Project or regen:CreditClass). And that we would actually need to define the constraints of ecosystem type on both the regen:Project and regen:CreditClass definitions.

Using something like OWL or RDFS (I think actually only OWL has the ability to express finite set of values for a property), one could actually define the property regen:ecosystemType and then separately say which classes can have this property.

Let me know if you think I'm misunderstanding something here @aaronc

So I think this depends on what we actually want to accomplish.

If we want to make it possible for users to look at our documentation and create compliant credit class or project graphs, then what we want to be doing is actually documenting the SHACL constraints for those graphs.

If we just want to descriptively talk about what properties and classes exist and what their potential domains and ranges are, then documenting that with RDFS and OWL (where necessary) makes sense.

I question though how much this descriptive use case actually serves our users. It might actually be more useful to have documentation of how to write JSON-LD that will pass our SHACL constraints. That's a very different undertaking. Ideally we would have a site that does both.

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.

5 participants