feat: use origin based </x/y/> relativisation rather than <../../../x/y/> when result is a shorter relative IRI.
#529
Conversation
WalkthroughThe changes update the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant BaseIRI
Caller->>BaseIRI: toRelative(targetIRI)
BaseIRI->>BaseIRI: _getBaseMatcher()
Note right of BaseIRI: Use `absoluteIris` flag and segment length<br>to decide path replacement strategy
BaseIRI-->>Caller: Returns relative or absolute path based on logic
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.38.1)test/N3Writer-test.js🔧 ESLint
npm error Exit handler never called! ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/BaseIRI.js(2 hunks)test/BaseIRI-test.js(1 hunks)
🔇 Additional comments (2)
test/BaseIRI-test.js (1)
144-148: LGTM! Good test coverage for origin-based path optimization.These test cases effectively validate the new logic that prefers shorter origin-based paths over longer parent-based paths.
src/BaseIRI.js (1)
9-9: LGTM! Good use of a named constant.Using a named constant for the origin character improves code readability and maintainability.
|
Do we want this though? Because a big reason for relative support in the writer is the ability to move documents without changing syntax. CC @smessie |
|
Although Therefore, I strongly recommend maintaining the current state and refraining from merging this PR. |
|
Right, that makes sense. The context for opening this PR is that I use the Given that I think this functionality would still be useful opt-in functionality in |
|
My two cents: pretty printing is a very different business from N3Writer. I'd architect the thing very differently for pretty printing (especially if the streaming constraint can be dropped). It could be an option on BaseIRI.js; wondering if it makes sense to forward base IRI options from N3Writer to BaseIRI. I'm mainly concerned about the slippery slope, in which N3Writer becomes no-actually-a-pretty-printer-but-some-features-are-getting-close-so-why-not, whereas the design goals are different. N3Writer is about writing Turtle fast (so any abbreviations are for speed more than readability), and features tend to serve a functional need (relative URIs are to facilitate moving). Pretty printers are inherently different beasts. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/N3Writer-test.js (1)
5667-5678: Well-implemented backward-compatible function signature enhancement.The function signature enhancement elegantly maintains backward compatibility while adding support for the options object. The logic correctly handles both the legacy string parameter and the new options object format.
However, consider adding parameter validation for the options object:
function testRelativizes(baseIRI, ...cases) { let absoluteIris = false; if (typeof baseIRI === 'object') { + if (!baseIRI.baseIRI) { + throw new Error('baseIRI property is required when passing options object'); + } absoluteIris = baseIRI.absoluteIris; baseIRI = baseIRI.baseIRI; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/BaseIRI.js(2 hunks)src/N3Writer.js(1 hunks)test/BaseIRI-test.js(1 hunks)test/N3Writer-test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/BaseIRI.js
- test/BaseIRI-test.js
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/N3Writer.js (1)
src/BaseIRI.js (1)
BaseIRI(14-109)
🔇 Additional comments (2)
src/N3Writer.js (1)
63-63: Clean implementation of opt-in feature.The change correctly forwards the
absoluteIrisoption to theBaseIRIconstructor, enabling the new origin-based relativisation as an opt-in feature. This addresses the concerns raised in the PR discussion about maintaining backward compatibility while providing the new functionality for use cases that prioritize compactness over document portability.The implementation maintains backward compatibility since
options.absoluteIriswill beundefinedwhen not provided, which preserves existing behavior.test/N3Writer-test.js (1)
5568-5665: Comprehensive test coverage for theabsoluteIrisfeature.The test suite effectively covers the new origin-based relativization functionality with a comprehensive set of test cases. The expected outputs correctly demonstrate the logic of using absolute paths (e.g.,
/foo,/foo/bar) when they're more concise and relative paths (e.g.,../,../extended) when they result in shorter IRIs.
Partially resolves #528 but does not cover the following test cases (where there are 2 or more sub paths after the relativisation):
Summary by CodeRabbit