-
Notifications
You must be signed in to change notification settings - Fork 0
ブラウザ版のrankingHistory関数で、fetchOptionsを考慮していなかったのを修正 #215
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
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Summary of ChangesHello @deflis, 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 enhances the 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. 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
このプルリクエストは、ブラウザ版の rankingHistory 関数で fetchOptions を渡せるようにする改善ですね。APIの柔軟性を高めるという目的は素晴らしいと思います。しかし、現在の実装では関数のシグネチャが変更されており、既存のコードとの後方互換性が失われる「破壊的変更」となってしまっています。これを修正するための提案をコメントに記載しました。また、コードの可読性をさらに向上させるための小さな提案も加えています。詳細については、各レビューコメントをご確認ください。
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.
Pull request overview
This pull request fixes a bug in the browser version of the rankingHistory function where the ExecuteOptions parameter was not being accepted or passed through to the underlying API call. This brings it into parity with the non-browser version of the same function.
Changes:
- Added
ExecuteOptionsimport from narou.ts to support fetch customization - Updated
rankingHistoryfunction signature to accept an optionaloptionsparameter - Modified the API call to pass the options through to
executeRankingHistory
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- ExecuteOptions型をインポート - api.executeRankingHistoryにoptionsを渡すよう修正
c1e467d to
ad02dae
Compare
|
/gemini review |
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
このプルリクエストは、ブラウザ版の rankingHistory 関数に fetchOptions を渡すための options パラメータが欠けていた問題を修正するものです。変更内容を確認しました。
主な変更点は src/index.browser.ts ファイルで、rankingHistory 関数のシグネチャに options?: ExecuteOptions を追加し、JSDocも更新しています。これにより、Node.js版とブラウザ版でAPIの互換性が保たれるようになります。
修正は適切であり、問題なく動作すると思われます。1点、JSDocの可読性をさらに向上させるための提案をコメントしました。
| * なろう殿堂入り API でランキング履歴を取得する | ||
| * @param {string} ncode 小説のNコード | ||
| * @param {ExecuteOptions} [options] 実行オプション | ||
| * @param {NarouNovel} [api] API実行クラスのインスタンス |
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.
#213 での考慮漏れ。
index.tsの関数シグネチャを変更したが、ブラウザ版index.browser.tsで関数にオプションを追加していなかったので、ブラウザでランキング履歴を取得するときだけ壊れるようになっていた。