Add xsd:decimal support to vocab runtime and codegen#640
Add xsd:decimal support to vocab runtime and codegen#640dahlia merged 5 commits intofedify-dev:mainfrom
xsd:decimal support to vocab runtime and codegen#640Conversation
Add the branded Decimal runtime type with isDecimal() and parseDecimal() so exact xsd:decimal values can be validated without introducing a decimal arithmetic dependency. Teach the vocabulary code generator to map xsd:decimal to Decimal, use the shared runtime predicate in generated guards, and decode typed literals through parseDecimal(). Update the vocabulary manual and changelog to document the new runtime APIs and the xsd:decimal code generation support. fedify-dev#617 Co-Authored-By: OpenAI Codex <codex@openai.com>
Summary of ChangesHello, 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 lays foundational groundwork for handling Highlights
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. Footnotes
|
issues-auto-labeler
bot
added
breaking change
|
@codex review |
![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 support for the xsd:decimal type, which is a crucial foundational change for handling precision-safe values like prices and measurements. The changes are well-structured, adding a new Decimal branded string type and validation helpers (isDecimal, parseDecimal) to @fedify/vocab-runtime, and updating the code generator in @fedify/vocab-tools to correctly handle this new type. The implementation is robust, with a correct regular expression for validation and comprehensive unit tests covering both valid and invalid lexical forms. The documentation in CHANGES.md and the manual has also been updated accordingly. The code quality is excellent, and I have no further suggestions for improvement.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ce09b5522f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Suppress no-unused-vars and verbatim-module-syntax in generated vocabulary files so the unconditional Decimal helper imports do not break linting before any schema actually uses xsd:decimal. Keep the generated snapshots in sync with the updated file header. Co-Authored-By: OpenAI Codex <codex@openai.com>
Rewrite the throwing parseDecimal() JSDoc example so it catches the expected TypeError instead of failing when doctests execute the snippet directly. Co-Authored-By: OpenAI Codex <codex@openai.com>
Codecov Report❌ Patch coverage is
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
Add canParseDecimal() alongside the strict isDecimal() guard so xsd:decimal parsing can follow XML Schema whitespace normalization without weakening the Decimal type guard. Update parseDecimal() to normalize XML Schema whitespace before validation, adjust generated decimal data checks to use canParseDecimal(), and refresh the related tests and documentation. fedify-dev#640 (comment) Co-Authored-By: OpenAI Codex <codex@openai.com>
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces support for xsd:decimal by adding a Decimal branded string type and associated helper functions (isDecimal, canParseDecimal, parseDecimal) to @fedify/vocab-runtime. The code generation in @fedify/vocab-tools is updated to use this new type for properties with an xsd:decimal range. The changes are well-implemented, with thorough JSDoc documentation and comprehensive unit tests. The documentation in docs/manual/vocab.md and the CHANGES.md file have also been updated accordingly. The implementation is solid and I have no specific feedback for improvement.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 05d793d481
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Reject property ranges that mix xsd:string and xsd:decimal during code generation, since both map to runtime strings and the generated encoder cannot disambiguate them reliably. Update the vocab-tools tests to cover the rejection path directly, and document the restriction in the manual and changelog. fedify-dev#640 (comment) Co-Authored-By: OpenAI Codex <codex@openai.com>
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces comprehensive support for the xsd:decimal type. A new Decimal branded string type and associated helper functions (isDecimal, canParseDecimal, parseDecimal) have been added to @fedify/vocab-runtime, complete with thorough unit tests. The code generation process in @fedify/vocab-tools has been updated to utilize this new type and to prevent schema ambiguities by disallowing properties that mix xsd:string and xsd:decimal. The changes are well-documented in both the changelog and the manual. The implementation is robust, well-tested, and aligns with the project's coding standards.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: af8de5a4d5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
sij411
left a comment
sij411
left a comment
There was a problem hiding this comment.
I tested the following edge cases locally against this branch and all
passed:
- Precision preservation for very long decimals —
parseDecimal("99999999999999999999.99999999999999999999") round-trips
correctly through parseDecimal, isDecimal, and canParseDecimal
without truncation or loss. - Empty string rejection via canParseDecimal — canParseDecimal("")
returns false. - Empty string rejection via isDecimal — isDecimal("") returns
false.
I didn't add the tests.
2 participants
This PR factors out the
xsd:decimalsupport needed by #579 into a separate foundational change, as discussed in #617, so the broader FEP-0837 vocabulary work in #578 can use a semantically correct and precision-safe type. It provides the runtime and code generation support first, without changing@fedify/vocabto usexsd:decimalyet. See also FEP-0837.Summary
Decimalas a public branded string type in@fedify/vocab-runtime.isDecimal()andparseDecimal()for validating and parsingxsd:decimallexical values.@fedify/vocab-toolsso properties with thexsd:decimalrange are generated asDecimal.isDecimal()and decoders to useparseDecimal().Why
The immediate motivation for this change came from the review of #579, where
Measure.hasNumericalValuehad to usexsd:stringas a stopgap because the vocabulary type system did not supportxsd:decimalyet. That workaround is safe in the sense that it avoids binary floating-point rounding problems, but it loses the semantic distinction between arbitrary strings and decimal literals and pushes validation onto downstream users.Supporting
xsd:decimaldirectly fixes that gap at the right layer. JavaScriptnumberis not suitable here becausexsd:decimalis expected to preserve decimal precision exactly, which matters for marketplace and monetary values in FEP-0837. A branded string representation keeps the exact lexical value intact at runtime, avoids adding a decimal arithmetic dependency, and still gives generated APIs a dedicated type instead of a plainstring.This also follows the direction discussed in #617: keep the runtime representation simple and dependency-free for now, but expose a clear
Decimaltype and parsing API so Fedify can grow into properxsd:decimalsupport incrementally. That makes this PR a small, focused prerequisite for the vocabulary work in #578 and #579 rather than mixing type-system groundwork with the vocabulary addition itself.Testing
pnpm --filter @fedify/vocab-runtime test.pnpm --filter @fedify/vocab-tools test.deno testin packages/vocab-runtime/.deno test -Ain packages/vocab-tools/.