Conversation
Summary of ChangesHello @gundermanc, 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 significantly improves type safety by introducing and enforcing the Highlights
Changelog
Activity
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
|
|
Size Change: +375 B (0%) Total Size: 25.2 MB ℹ️ View Unchanged
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a great step towards improving type safety by enabling the @typescript-eslint/no-unsafe-return ESLint rule. My review focuses on areas where the newly flagged errors can be resolved with simple type assertions or annotations rather than just suppressing the rule. These changes will enhance the overall type safety and clarity of the code.
| // eslint-disable-next-line @typescript-eslint/no-unsafe-return | ||
| return response.confirm; |
There was a problem hiding this comment.
Instead of disabling the no-unsafe-return rule, you can ensure type safety by explicitly converting the returned value to a boolean. The response.confirm property from the prompts library is of type any, but this function is expected to return a boolean. Using !! is a concise way to guarantee a boolean return value.
| // eslint-disable-next-line @typescript-eslint/no-unsafe-return | |
| return response.confirm; | |
| return !!response.confirm; |
| // eslint-disable-next-line @typescript-eslint/no-unsafe-return | ||
| return results.map((r: { item: RegistryExtension }) => r.item); |
There was a problem hiding this comment.
Instead of disabling the no-unsafe-return rule, you can fix the underlying type issue. The results variable is inferred as any due to the no-unsafe-assignment suppression. By casting results to its expected type, you can ensure type safety for the return value and remove the need for this suppression.
| // eslint-disable-next-line @typescript-eslint/no-unsafe-return | |
| return results.map((r: { item: RegistryExtension }) => r.item); | |
| return (results as Array<{ item: RegistryExtension }>).map((r) => r.item); |
| // eslint-disable-next-line @typescript-eslint/no-unsafe-return | ||
| return JSON.parse(content); |
There was a problem hiding this comment.
Instead of suppressing the no-unsafe-return error, it's better to explicitly cast the result of JSON.parse to the expected return type AllExtensionsEnablementConfig. This improves type safety and makes the code clearer.
| // eslint-disable-next-line @typescript-eslint/no-unsafe-return | |
| return JSON.parse(content); | |
| return JSON.parse(content) as AllExtensionsEnablementConfig; |
| // eslint-disable-next-line @typescript-eslint/no-unsafe-return | ||
| return response.value; |
There was a problem hiding this comment.
Instead of suppressing the no-unsafe-return error, you can ensure type safety by casting the any value from the prompts library to the expected string type.
| // eslint-disable-next-line @typescript-eslint/no-unsafe-return | |
| return response.value; | |
| return response.value as string; |
| // eslint-disable-next-line @typescript-eslint/no-unsafe-return | ||
| return results.map( | ||
| (result: { item: ResourceSuggestionCandidate }) => result.item.suggestion, | ||
| ); |
There was a problem hiding this comment.
Instead of disabling the no-unsafe-return rule, you can fix the underlying type issue. The results variable is inferred as any. By casting results to its expected type, you can ensure type safety for the return value and remove the need for this suppression.
return (results as Array<{ item: ResourceSuggestionCandidate }>).map(
(result) => result.item.suggestion,
);| // eslint-disable-next-line @typescript-eslint/no-unsafe-return | ||
| return JSON.parse( | ||
| this.cleanJsonResponse(getResponseText(result)!.trim(), model), | ||
| ); |
There was a problem hiding this comment.
Instead of suppressing the no-unsafe-return error, it's better to explicitly cast the result of JSON.parse to the expected Record<string, unknown> type. This improves type safety.
return JSON.parse(
this.cleanJsonResponse(getResponseText(result)!.trim(), model),
) as Record<string, unknown>;| // eslint-disable-next-line @typescript-eslint/no-unsafe-return | ||
| return parsedJson.content; |
There was a problem hiding this comment.
Instead of suppressing the no-unsafe-return error, you can ensure type safety by casting parsedJson.content to a string. The parsedJson object is of type any, so this cast clarifies the intended return type.
| // eslint-disable-next-line @typescript-eslint/no-unsafe-return | |
| return parsedJson.content; | |
| return parsedJson.content as string; |
| // eslint-disable-next-line @typescript-eslint/no-unsafe-return | ||
| return JSON.parse(portFileContents); |
There was a problem hiding this comment.
Instead of suppressing the no-unsafe-return error, it's better to explicitly cast the result of JSON.parse to the function's expected return type. This improves type safety.
| // eslint-disable-next-line @typescript-eslint/no-unsafe-return | |
| return JSON.parse(portFileContents); | |
| return JSON.parse(portFileContents) as ConnectionConfig & { workspacePath?: string; ideInfo?: IdeInfo }; |
| // eslint-disable-next-line @typescript-eslint/no-unsafe-return | ||
| return JSON.parse(content); |
There was a problem hiding this comment.
Instead of suppressing the no-unsafe-return error, it's better to explicitly cast the result of JSON.parse to the expected type within the map. This improves type safety for the parsedContents array, which will in turn resolve the other no-unsafe-return issues later in this function.
| // eslint-disable-next-line @typescript-eslint/no-unsafe-return | |
| return JSON.parse(content); | |
| return JSON.parse(content) as (ConnectionConfig & { workspacePath?: string; ideInfo?: IdeInfo }); |
| // eslint-disable-next-line @typescript-eslint/no-unsafe-return | ||
| return JSON.parse(this.cachedLastConvData); |
There was a problem hiding this comment.
Instead of suppressing the no-unsafe-return error, it's better to explicitly cast the result of JSON.parse to the expected ConversationRecord type. This improves type safety.
| // eslint-disable-next-line @typescript-eslint/no-unsafe-return | |
| return JSON.parse(this.cachedLastConvData); | |
| return JSON.parse(this.cachedLastConvData) as ConversationRecord; |
gemini-cli
bot
added
the
area/core
2 participants
Summary
Disallow unsafe returns (returns with any type).
Fixes: #19720