Add vocabulary relating to proposals, intentions and measures. #579
Add vocabulary relating to proposals, intentions and measures. #579scammo wants to merge 10 commits intofedify-dev:mainfrom
Conversation
Summary of ChangesHello @scammo, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
issues-auto-labeler
bot
added
activitypub/interop
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces new vocabulary types for economic resource coordination, specifically Proposal, Intent, and Measure, based on FEP-0837. This is a great addition that will enable building federated marketplace applications with Fedify.
The implementation is well-done, with new YAML definitions for the vocabulary generator and corresponding test snapshot updates. I have a few minor suggestions to improve the changes:
- In
CHANGES.md, please add your name to the changelog entry as per the contribution guidelines and remove an unused reference link. - In
packages/vocab/src/measure.yaml, thehasNumericalValueproperty should use a numeric type (xsd:float) instead ofxsd:stringfor better type safety and interoperability.
Thank you for your contribution, especially as a first-time contributor! These changes are valuable to the project.
# Conflicts: # CHANGES.md # packages/vocab-tools/src/__snapshots__/class.test.ts.deno.snap # packages/vocab-tools/src/__snapshots__/class.test.ts.node.snap # packages/vocab-tools/src/__snapshots__/class.test.ts.snap
|
FYI: I updated the comments, CHANGES.md and the snapshots to the current version :) |
Codecov Report✅ All modified and coverable lines are covered by tests. 🚀 New features to boost your workflow:
|
|
FYI: I resolved the conversation and I created a new enhencement issue #617 |
| name: Proposal | ||
| compactName: Proposal | ||
| uri: "https://w3id.org/valueflows/ont/vf#Proposal" | ||
| extends: "https://www.w3.org/ns/activitystreams#Object" |
There was a problem hiding this comment.
According to FEP-0837, Proposal is a pure ValueFlows type, not an ActivityStreams Object. However, I see you've set extends: "https://www.w3.org/ns/activitystreams#Object" here.
This does have a practical benefit: inheriting properties like name, content, attributedTo, published, to, and location that the spec recommends for Proposal. So it may be an intentional design choice. Could you clarify the reasoning?
If the intent is to reuse AS properties for interoperability, that's reasonable, but it should be documented (e.g., in the description) so future maintainers understand this is a deliberate deviation from the spec rather than an oversight.
| $schema: ../../vocab-tools/schema.yaml | ||
| name: Intent | ||
| compactName: Intent | ||
| uri: "https://w3id.org/valueflows/ont/vf#Intent" |
There was a problem hiding this comment.
Intent is marked as entity: true, which means it can be independently fetched by URI. In FEP-0837, Intents are typically embedded within a Proposal and use fragment URIs (e.g., #primary, #reciprocal), which suggests they function more as embedded value objects.
Is there a use case where an Intent would be fetched independently outside of its parent Proposal? If not, entity: false might be more appropriate, similar to how Measure is defined.
Also, unlike Proposal, Intent has no extends field. If Proposal extends Object for practical reasons (reusing name, content, etc.), should Intent also extend Object for consistency, or is the asymmetry intentional?
| Proposal: "vf:Proposal" | ||
| Intent: "vf:Intent" | ||
| purpose: "vf:purpose" | ||
| unitBased: "vf:unitBased" | ||
| publishes: "vf:publishes" | ||
| reciprocal: "vf:reciprocal" | ||
| action: "vf:action" | ||
| resourceConformsTo: | ||
| "@id": "vf:resourceConformsTo" | ||
| "@type": "@id" | ||
| resourceQuantity: "vf:resourceQuantity" | ||
| availableQuantity: "vf:availableQuantity" | ||
| minimumQuantity: "vf:minimumQuantity" | ||
| hasUnit: "om2:hasUnit" | ||
| hasNumericalValue: "om2:hasNumericalValue" |
There was a problem hiding this comment.
The defaultContext here includes mappings for Intent, action, resourceConformsTo, hasUnit, hasNumericalValue, etc. These are properties that belong to Intent and Measure, not to Proposal itself.
I understand this is likely to ensure correct JSON-LD serialization when Intent/Measure objects are embedded. However, each of those types already defines its own defaultContext. Could you verify that the code generator handles nested contexts correctly without this duplication? If the duplication is necessary, a comment explaining why would be helpful.
| The amount of the resource. If not specified, arbitrary amounts can be | ||
| used when responding to the proposal. | ||
| range: | ||
| - "http://www.w3.org/2001/XMLSchema#string" |
There was a problem hiding this comment.
I see this was already discussed and xsd:string was agreed upon as a pragmatic stopgap until xsd:decimal support is added (#617). That's fine for now.
One small suggestion: it would be helpful to add a note in the description field mentioning that xsd:string is a temporary choice and that the value should represent a decimal number. This helps consumers of the API understand the expected format. For example:
description: |
The amount of the resource, represented as a decimal string. If not
specified, arbitrary amounts can be used when responding to the proposal.
NOTE: This property uses `xsd:string` as a temporary measure until
`xsd:decimal` is supported. See #617.|
|
||
| ### @fedify/vocab | ||
|
|
||
| - Added vocabulary types for economic resource coordination | ||
| in federated networks. [[#578]] | ||
|
|
||
| - Added `Proposal` class for publishing offers or requests. | ||
| - Added `Intent` class for describing economic transactions within | ||
| a proposal, with `action`, `resourceConformsTo`, `resourceQuantity`, | ||
| `availableQuantity`, and `minimumQuantity` properties. | ||
| - Added `Measure` class for representing quantities with units of | ||
| measure, with `hasUnit` and `hasNumericalValue` properties. | ||
|
|
||
| [#578]: https://github.com/fedify-dev/fedify/issues/578 | ||
| [ValueFlows]: https://www.valueflo.ws/ |
There was a problem hiding this comment.
These entries appear under the Version 1.5.0 section, which has already been released. Since this PR hasn't been merged yet, the changelog entry should only appear in the unreleased section at the top. Please remove this duplicate block.
Also, the [#578] reference link is defined twice (once in each section), and the [ValueFlows] reference link is defined but never used. Please clean these up.
| - Added vocabulary types for economic resource coordination | ||
| in federated networks. [[#578] by scammo] | ||
| - Added `Proposal` class for publishing offers or requests. | ||
| - Added `Intent` class for describing economic transactions within | ||
| a proposal, with `action`, `resourceConformsTo`, `resourceQuantity`, | ||
| `availableQuantity`, and `minimumQuantity` properties. |
There was a problem hiding this comment.
The sub-items (Proposal, Intent, Measure) should be nested under the summary line rather than being flat siblings. Also, please run mise run fmt to fix Hongdown formatting issues.
Suggested structure:
- Added vocabulary types for economic resource coordination
in federated networks. [[#578] by Samuel Brinkmann]
- Added `Proposal` class for publishing offers or requests.
- Added `Intent` class for describing economic transactions within
a proposal, with `action`, `resourceConformsTo`, `resourceQuantity`,
`availableQuantity`, and `minimumQuantity` properties.
- Added `Measure` class for representing quantities with units of
measure, with `hasUnit` and `hasNumericalValue` properties.2 participants
Summary
Add vocabulary relating to proposals, intentions and measures according to the docs.
Related issue
Reference the related issue(s) by number, e.g.:
Changes
Benefits
Users can now build a federated system based on the first part of FEP-0837: Federated Marketplace (https://codeberg.org/fediverse/fep/src/branch/main/fep/0837/fep-0837.md). This is really interesting for making offered goods or services available for discovery in the Fediverse.
Checklist
mise teston your machine?Additional notes
Please note that this is both my first pull request and my first enhancement issue for this project. While I have extensive Mastodon/event experience, I have no prior experience of enhancing federation tooling directly. I am happy to make any necessary changes or additions. Thank you for your work!