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

refactor!: use schema:ItemList consistently for regen:approvedMethodologies #40

Merged
merged 4 commits into from
Mar 13, 2023

Conversation

wgwz
Copy link
Contributor

@wgwz wgwz commented Feb 7, 2023

Description

Closes: #33


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)

@wgwz wgwz requested a review from blushi February 7, 2023 23:04
@wgwz wgwz mentioned this pull request Feb 7, 2023
6 tasks
Base automatically changed from kyle/1420-project-metadata-separation to main February 8, 2023 14:47
@wgwz
Copy link
Contributor Author

wgwz commented Feb 21, 2023

In addition to needing some reviews, we are waiting to merge this because it will require updating credit classes on-chain.

}
],
"regen:approvedMethodologiesURL": {
Copy link
Member

Choose a reason for hiding this comment

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

As discussed together as second option, what about using ItemList instead? Then we can define some schema:url directly on it instead of creating our ownregen:approvedMethodologiesURL
I believe it'd look cleaner

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sounds good to me, i'll push a commit making that change

Copy link
Contributor Author

@wgwz wgwz Feb 27, 2023

Choose a reason for hiding this comment

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

one thing to note is that we will need to make updates in the bridge service for this change, since it changes the format of the regen:approvedMethodologies field. toucan projects use that field and so we'll need to update that there.

updates to the bridge service would be located around here:

Copy link
Member

@blushi blushi Feb 28, 2023

Choose a reason for hiding this comment

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

yeah I can create an issue to address that on the bridge side once this is merged, we'll need to update the front-end too since it assumesregen:approvedMethodologies to be a @list for now (which is why the approved methodologies are not displayed on the credit class page cc/ @clevinson ) and set up txs to update credit classes cc/ @S4mmyb

@blushi blushi requested a review from S4mmyb February 28, 2023 08:25
@blushi
Copy link
Member

blushi commented Mar 1, 2023

@wgwz could you update the PR title, now that we're going for schema:ItemList rather than dash:ListShape? thanks

@blushi blushi requested a review from clevinson March 1, 2023 11:04
@wgwz wgwz changed the title refactor!: use dash:ListShape instead of schema:BreadcrumbList refactor!: use schema:ItemList instead of dash:ListShape and schema:BreadcrumbList Mar 1, 2023
@wgwz wgwz changed the title refactor!: use schema:ItemList instead of dash:ListShape and schema:BreadcrumbList refactor!: use schema:ItemList consistently for regen:approvedMethodologies Mar 1, 2023
@@ -42,332 +36,285 @@
"Carbon Removals"
],
"regen:approvedMethodologies": {
"@type": "schema:BreadcrumbList",
"@type": "schema:ItemList",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@blushi just wanted to point the adjustments to C03 credit class. it's just to conform the approvedMethodologies to schema:ItemList.

sh:minCount 1 ;
sh:maxCount 1 ;
] ;
sh:property regen:ApprovedMethodologiesPropertyShape ;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@blushi some more adjustments for the C03 credit class here. we're using the same property shape for all instances of regen:approvedMethodologies

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.

looks good, pre-approving, just a few comments

Comment on lines +79 to +80
# TODO: eventually this should set on most of our shapes
# this will help us catch missing fields
Copy link
Member

Choose a reason for hiding this comment

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

should we create an issue to address that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#41

Copy link
Member

Choose a reason for hiding this comment

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

What's the blocker on adding this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nothing really blocking it, it probably would just involve a little bit of clean-up and i'd prefer to handle it separately from this

Copy link
Member

@ryanchristo ryanchristo left a comment

Choose a reason for hiding this comment

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

LGTM. Just a couple questions.

Comment on lines +9 to +11
"schema:url": {
"@type": "schema:URL"
},
Copy link
Member

Choose a reason for hiding this comment

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

The expected value of schema:url is URL. I don't think need this in the same way we don't need schema:name and schema:description with type Text in the context. Is there another reason for including this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

schema:name and schema:description are different.
we used to use schema:Text for fields like that.
but then we decided to move away from schema:Text.
when we were using schema:Text we had to be explicit about those types.
just like we need to be explicit about the type here.
the reason we no longer need a type specified for schema:name and schema:description is that we agreed they can be xsd:string types instead.
when JSON-LD documents are parsed, JSON string values are implicitly assumed to be of the xsd:string type.
because we do want "schema:url" to be interpreted as a URL type and not an xsd:string, we need to add that type info in the context.
otherwise schema:url would also be implicitly assumed to be an xsd:string value

Comment on lines +79 to +80
# TODO: eventually this should set on most of our shapes
# this will help us catch missing fields
Copy link
Member

Choose a reason for hiding this comment

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

What's the blocker on adding this?

@wgwz wgwz merged commit 3a209f0 into main Mar 13, 2023
@wgwz wgwz deleted the kyle/33-simplify-lists branch March 13, 2023 18:41
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.

Remove the BreadcrumbList's in favor dashlist shapes
3 participants