-
Notifications
You must be signed in to change notification settings - Fork 46
[FirebaseAI] Fix minor issues with reference docs #1251
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: main
Are you sure you want to change the base?
Conversation
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.
Hello @a-maurice, 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!
Summary of Changes
This pull request focuses on improving the generated reference documentation for the FirebaseAI library. It addresses two main issues: hiding the internal namespace from the generated documentation and modifying properties to use explicit getter syntax instead of lambda notation, which was causing problems with the documentation generation process. The changes involve updates to the CMake build configuration to exclude internal files from documentation generation and modifications to several C# files to change property definitions.
Highlights
- Documentation Generation: The primary goal is to improve the quality and accuracy of the generated reference documentation for the FirebaseAI library.
- Internal Namespace Hiding: The pull request ensures that the
Internal
namespace is excluded from the generated documentation, preventing internal implementation details from being exposed to users. - Property Syntax Modification: Properties are changed to use explicit getter syntax instead of lambda notation to ensure correct documentation generation.
Changelog
Click here to see the changelog
- firebaseai/CMakeLists.txt
- Added a filter to exclude files in the
src/Internal/
directory from the documentation generation process.
- Added a filter to exclude files in the
- firebaseai/src/Candidate.cs
- Modified the
SafetyRatings
property to use explicit getter syntax instead of lambda notation.
- Modified the
- firebaseai/src/Chat.cs
- Modified the
History
property to use explicit getter syntax instead of lambda notation.
- Modified the
- firebaseai/src/Citation.cs
- Modified the
Citations
property to use explicit getter syntax instead of lambda notation.
- Modified the
- firebaseai/src/CountTokensResponse.cs
- Modified the
PromptTokensDetails
property to use explicit getter syntax instead of lambda notation.
- Modified the
- firebaseai/src/FirebaseAI.cs
- Modified the
DefaultInstance
property to use explicit getter syntax instead of lambda notation.
- Modified the
- firebaseai/src/GenerateContentResponse.cs
- Modified the
Candidates
property to use explicit getter syntax instead of lambda notation. - Modified the
SafetyRatings
property to use explicit getter syntax instead of lambda notation. - Modified the
PromptTokensDetails
property to use explicit getter syntax instead of lambda notation. - Modified the
CandidatesTokensDetails
property to use explicit getter syntax instead of lambda notation.
- Modified the
- firebaseai/src/LiveSessionResponse.cs
- Modified the
FunctionCalls
property to use explicit getter syntax instead of lambda notation. - Modified the
FunctionIds
property to use explicit getter syntax instead of lambda notation.
- Modified the
- firebaseai/src/ModelContent.cs
- Modified the
Role
property to use explicit getter syntax instead of lambda notation. - Modified the
Parts
property to use explicit getter syntax instead of lambda notation.
- Modified the
- firebaseai/src/Schema.cs
- Modified the
EnumValues
property to use explicit getter syntax instead of lambda notation. - Modified the
RequiredProperties
property to use explicit getter syntax instead of lambda notation. - Modified the
PropertyOrdering
property to use explicit getter syntax instead of lambda notation. - Modified the
AnyOfSchemas
property to use explicit getter syntax instead of lambda notation.
- Modified the
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command>
or @gemini-code-assist <command>
. Below is a summary of the supported commands.
Feature | Command | Description |
---|---|---|
Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
Help | /gemini help |
Displays a list of available commands. |
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 .gemini/
folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and 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 to provide feedback.
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.
A doc's clarity,
A guide for all to see,
Code's best friend, true.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
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 addresses issues with reference documentation generation by changing C# properties from lambda notation (expression-bodied members) to standard get-accessor properties. Additionally, it hides the Internal
namespace from the generated documentation by updating the CMakeLists.txt
file.
The changes are clear, directly address the stated problem, and the reasoning for moving away from expression-bodied members (tooling compatibility for docs) is well-understood. The update to CMakeLists.txt
effectively filters out internal implementation details from the public API documentation.
One minor point regarding process: the "Testing" section in the PR description is still a placeholder. It would be beneficial to describe how these changes were verified, presumably by generating the reference documentation and checking for the corrected property rendering and exclusion of internal members.
Summary of Findings
- Stylistic Consistency in Property Getters: There's a minor stylistic inconsistency in how simple getters are formatted. Some simple getters (e.g., in
Schema.cs
) use a compact single-line format likeget { return _value; }
, while others with similarly simple logic (e.g.,FirebaseAI.DefaultInstance
) use a multi-line format. While both are valid, adopting a consistent style for simple one-line getters could improve uniformity. This is a low-severity observation and not commented on directly due to review settings. - Minor Formatting in
Chat.cs
: Infirebaseai/src/Chat.cs
, theHistory
property's getter has an extra leading space before thereturn
statement (return new ReadOnlyCollection<ModelContent>(chatHistory);
). This is a very minor formatting detail. This is a low-severity observation and not commented on directly due to review settings.
Merge Readiness
The changes in this pull request are well-justified and correctly implemented to fix issues with reference documentation generation. There are no critical or high-severity issues identified. The minor stylistic points noted in the findings summary are of low severity and do not block merging.
Based on this review, the code appears to be in good shape to be merged once the testing details are filled in, if required by your process. As a reviewer, I am not authorized to approve the pull request, so please ensure it undergoes any further necessary reviews and approvals before merging.
Description
Change properties to not use lambda notation, since the generated ref docs weren't handling them well. Add comments to ResponseModality enum.
Testing
Looking at the generated ref docs
Type of Change
Place an
x
the applicable box: