-
-
Notifications
You must be signed in to change notification settings - Fork 84
feat: RFC 6570 Compatible @fedify/uri-template Package
#475
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
base: next
Are you sure you want to change the base?
Conversation
@fedify/uri-template Package
Summary of ChangesHello @notJoon, 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 introduces URI template support, restores a previous implementation, and includes significant updates to the Fedify ecosystem. These changes involve restructuring packages, updating dependencies, enhancing CLI tooling, and modifying default configurations to improve modularity, developer experience, and compliance with ActivityPub standards. Highlights
Ignored Files
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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a new, RFC 6570-compliant URI template package, @fedify/uri-template. The implementation is thorough, covering expansion and symmetric matching with different encoding policies. The code is well-structured and comes with comprehensive documentation and tests. However, I've identified a critical issue in the matching logic that needs to be addressed, along with several medium to high severity issues related to package configuration, documentation, and examples that impact usability and maintainability.
| if (spec.first) { | ||
| // Operators starting with a distinct char must appear in URL | ||
| if (!readLiteral(spec.first)) return null; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The matching logic incorrectly handles expressions that can be empty. It unconditionally consumes the operator's first character (e.g., ., /, ;), which is incorrect for expressions that can resolve to an empty string, where the first character should not be present. For example, a template X{.foo}Y should match the URL XY when foo is undefined, but the current logic will fail because it expects a . before Y. This is a critical bug that breaks matching for many valid cases. The logic needs to be updated to handle the optionality of the first character based on whether the expression's variables produce any output.
Co-authored-by: Hong Minhee (洪 民憙) <[email protected]>
dahlia
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add packages/uri-template to the packages list in the pnpm-workspace.yaml file?
@dahlia I'll add it right away. also, I haven't updated CHANGES.md yet, would it be better to add it to version 1.10.0 or to version 2.0.0? |
|
You should add it to version 2.0.0! |
got it! now I updated pnpm-workspace.yaml, pnpm-lock.yaml and CHANGES.md files. 7dbe61c |
Summary
restored PR #421
Related Issue
partially resolve #418
Checklist
deno task test-allon your machine?