Skip to content

Implement actionable TODOs: typed errors, term type checks, lang string direction#32

Closed
Copilot wants to merge 6 commits intomainfrom
copilot/implement-open-todos-check-term-type
Closed

Implement actionable TODOs: typed errors, term type checks, lang string direction#32
Copilot wants to merge 6 commits intomainfrom
copilot/implement-open-todos-check-term-type

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Mar 20, 2026

Implements the open TODOs that don't require design decisions, covering typed error classes, term type guards in ValueMapping, RDF 1.2 directional language string support, and the missing test coverage they called for.

Typed error classes

Three exported error classes replace anonymous Error throws:

  • ValueMappingError — thrown when a value mapping receives the wrong termType
  • SingularNoValueError — thrown by singular() when no quad matches
  • SingularTooManyValuesError — thrown by singular() when multiple quads match

All three are exported from mod.ts.

ValueMapping term type checks

Every mapping function now guards on termType before converting:

// Before: silently returned wrong value or threw a cryptic runtime error
export function literalToDate(termWrapper: TermWrapper): Date {
    return new Date(termWrapper.value)
}

// After
export function literalToDate(termWrapper: TermWrapper): Date {
    if (termWrapper.termType !== "Literal") {
        throw new ValueMappingError("Literal", termWrapper.termType)
    }
    return new Date(termWrapper.value)
}

Affected functions: blankNodeToString, literalToDate, literalToLangString, literalToNumber, literalToBoolean, literalToString, iriToString.

RDF 1.2 directional language strings

The N3.js bug referenced in langStringToLiteral is fixed in v2.0.3. literalToLangString now populates direction and langStringToLiteral passes { language, direction } to the factory:

// literalToLangString now returns direction
return { direction: termWrapper.direction ?? '', lang: termWrapper.language, string: termWrapper.value }

// langStringToLiteral now uses the proper factory overload
factory.literal(value.string, { language: value.lang, direction: value.direction ?? '' })

Tests

All implemented TODOs in term_wrapper.test.ts are resolved:

  • Singular error tests assert on SingularNoValueError / SingularTooManyValuesError specifically
  • New describe("Set") block under Arity Mapping covers set cardinality behaviour
  • Nullable object mapping (Child | undefined) tested via new hasNullableChild property on the Parent model
  • Primitive-type wrapping set tested via new hasStringSet: Set<string> property
  • ValueMapping "Type Errors" subsection covers all ValueMappingError throw paths
  • Round-trip tests for direction: 'rtl' and direction: 'ltr' assert that literalToLangString preserves the RDF 1.2 direction value
  • Regression test for the N3.js bug (factory.literal(text, { language }) object form): calls TermMapping.langStringToLiteral and asserts the resulting term's language is set correctly, guarding against any future N3.js regression in object-form language handling
Original prompt

Please implement open 'todos' in this codebase that do not require a design decision -- such as "Check term type."

For each 'todo' implemented please make sure that appropriate test(s) are also written

Created from VS Code.


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

…ection, and missing tests

Co-authored-by: jeswr <63333554+jeswr@users.noreply.github.com>
Agent-Logs-Url: https://github.com/rdfjs/wrapper/sessions/035a1c66-f819-453e-8236-8513bc7eaf1c
Copilot AI changed the title [WIP] Implement open todos without design decisions Implement actionable TODOs: typed errors, term type checks, lang string direction Mar 21, 2026
Copilot AI requested a review from jeswr March 21, 2026 00:07
Comment on lines -16 to -21
// TODO: Lang string dictionary value mapping
export function langStringToLiteral(value: ILangString, dataset: DatasetCore, factory: DataFactory): TermWrapper | undefined {
// TODO: Fix bug in N3.JS function literal line 340 (test languageOrDataType === 'string')
// Then re-establish proper factory method
//return new TermWrapper(factory.literal(value.string, { language: value.lang } ), dataset, factory)
return new TermWrapper(factory.literal(value.string, value.lang), dataset, factory)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@langsamu could you please check that you are satisfied that these TODO's are resolved by this PR -- they are the only ones I am not sure on

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Note that I have asked co-pilot to perform a regression test in 04994ce -- can you please confirm that this means the N3.js bug is indeed fixed.

@jeswr jeswr marked this pull request as ready for review March 21, 2026 00:33
Copilot AI review requested due to automatic review settings March 21, 2026 00:33
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Implements previously-marked TODOs in the RDF/JS wrapper library by introducing typed error classes, enforcing termType guards in ValueMapping, and adding RDF 1.2 directional language-string support across term/value mappings and tests.

Changes:

  • Added exported typed errors for singular-arity failures and value-mapping term-type mismatches.
  • Implemented termType checks across ValueMapping conversions and updated tests accordingly.
  • Updated lang-string mappings to round-trip RDF 1.2 direction and expanded unit test coverage for several TODO areas.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
test/unit/vocabulary/Example.ts Adds new example predicates used by new nullable/set mapping tests.
test/unit/term_wrapper.test.ts Updates/extends tests for typed errors, sets, nullable object mapping, and lang-string direction shape.
test/unit/model/Parent.ts Adds hasNullableChild and hasStringSet mappings to support new tests.
src/mod.ts Re-exports newly introduced error classes as part of the public API.
src/mapping/ValueMapping.ts Adds termType guards and throws ValueMappingError; returns lang-string direction.
src/mapping/TermMapping.ts Uses RDF 1.2 lang-string factory overload including { language, direction }.
src/ValueMappingError.ts Introduces typed error for value-mapping term-type mismatches.
src/TermWrapper.ts Replaces generic singular() errors with typed singular-arity errors.
src/SingularTooManyValuesError.ts Introduces typed error for singular() when multiple values exist.
src/SingularNoValueError.ts Introduces typed error for singular() when no values exist.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/mapping/TermMapping.ts
@jeswr
Copy link
Copy Markdown
Collaborator

jeswr commented Mar 21, 2026

@copilot apply changes based on the comments in this thread

Copy link
Copy Markdown
Contributor Author

Copilot AI commented Mar 21, 2026

@copilot apply changes based on the comments in this thread

Added round-trip tests for direction: 'rtl' and direction: 'ltr' in the Term Mapping section (0df8f53). Both tests set an ILangString with the respective direction and assert literalToLangString returns the same value.

Copilot AI requested a review from jeswr March 21, 2026 00:39
@jeswr
Copy link
Copy Markdown
Collaborator

jeswr commented Mar 22, 2026

@copilot please add a test to make sure that the bug for which the following comment has been removed "TODO: Fix bug in N3.JS function literal line 340 (test languageOrDataType === 'string')" has actually been resolved.

Looking at the changelog of N3.js; I think that a different error was being fixed there.

Copy link
Copy Markdown
Contributor Author

Copilot AI commented Mar 22, 2026

@copilot please add a test to make sure that the bug for which the following comment has been removed "TODO: Fix bug in N3.JS function literal line 340 (test languageOrDataType === '...

Added a regression test (04994ce) that calls TermMapping.langStringToLiteral({ direction: "", lang: "en", string: "hello" }, ...) and asserts termWrapper.language === "en". This directly exercises the factory.literal(text, { language, direction }) object form that was previously blocked by the N3.js bug (test languageOrDataType === 'string'). If N3.js regresses the object-form handling, this test will fail.

Copy link
Copy Markdown
Collaborator

@jeswr jeswr left a comment

Choose a reason for hiding this comment

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

@langsamu I am happy with this PR, could you please take a quick glance before merging -- especially on the item I have tagged you in.

@matthieubosquet
Copy link
Copy Markdown
Collaborator

Errors have been done elsewhere and the mapping API has evolved.

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.

4 participants