Conversation
…ction proxy handling
…seBool function, and add tests for getFields
…mprove test setup
…ields function with improved logging
…ution and improve debug file output
Summary of ChangesHello @iugo, 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 expands the project's capabilities by integrating a new feature to fetch employee field configurations from the WeChat Work API. Concurrently, it refines the development workflow and code quality through the standardization of code style, comprehensive Deno configuration for linting and formatting, and the establishment of a more robust logging and debugging infrastructure. These changes collectively aim to improve developer experience, maintain code consistency, and prepare the codebase for future API extensions. 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
|
![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 adds functionality for fetching employee fields from the WeChat Work API, along with related configurations and helper utilities for logging and constants. The changes are a good step towards building out the SDK's features.
My review includes several points:
- A critical bug in how the proxy URL is constructed.
- Missing error handling for network requests and lack of assertions in tests, which are high-severity issues affecting correctness and reliability.
- Suggestions to improve performance by using asynchronous I/O for logging.
- Several medium-severity suggestions for improving code consistency and maintainability, such as standardizing comment languages.
Overall, the code is well-structured, but addressing these points will significantly improve its robustness and quality.
| }: SDKOptions): Promise<QywechatRes<{ group_list: FieldGroup[] }>> { | ||
| const domain = 'qyapi.weixin.qq.com'; | ||
| const path = '/cgi-bin/hr/get_fields'; | ||
| const url = new URL(`https://${proxy ?? ''}${domain}${path}`); |
There was a problem hiding this comment.
The current URL construction with the proxy seems incorrect. It concatenates the proxy and the domain (https://${proxy ?? ''}${domain}${path}), which will result in an invalid URL like https://myproxy.com/qyapi.weixin.qq.com/.... The proxy should likely replace the domain if provided.
| const url = new URL(`https://${proxy ?? ''}${domain}${path}`); | |
| const url = new URL(`https://${proxy ?? domain}${path}`); |
| Deno.test('getFields', async () => { | ||
| const res = await getFields({ accessToken, proxy }); | ||
| debug('getFields res:', res); | ||
| }); |
There was a problem hiding this comment.
This test executes getFields but lacks assertions to verify the response is correct. A test should validate the function's output.
I'd suggest:
- Adding assertions to check for a successful response (
errcode: 0) and that thegroup_listdata exists. - Awaiting the
debugcall, assuming you make it asynchronous to avoid blocking I/O as suggested in another comment forsrc/log.ts.
To use assertEquals and assertExists, you'll need to import them:
import { assertEquals, assertExists } from '@std/assert';| Deno.test('getFields', async () => { | |
| const res = await getFields({ accessToken, proxy }); | |
| debug('getFields res:', res); | |
| }); | |
| Deno.test('getFields', async () => { | |
| const res = await getFields({ accessToken, proxy }); | |
| await debug('getFields res:', res); | |
| assertEquals(res.errcode, 0); | |
| assertExists(res.group_list); | |
| }); |
| const res = await fetch(url); | ||
| const data = await res.json(); |
There was a problem hiding this comment.
The fetch call does not check if the response was successful (e.g., HTTP status 200-299). If the server returns an error status like 404 or 500, res.json() might fail or you might return an unexpected structure. It's good practice to check res.ok and handle non-successful responses explicitly.
const res = await fetch(url);
if (!res.ok) {
throw new Error(`Failed to get fields: ${res.status} ${await res.text()}`);
}
const data = await res.json();| * 设置 | ||
| * @param value | ||
| */ |
There was a problem hiding this comment.
The JSDoc for setVerbose is in Chinese, while the function name and other comments in this file are in English. For consistency and better maintainability, it's recommended to use English for all documentation within this file.
/**
* Sets the verbose flag.
* @param value
*/| const path = '/cgi-bin/hr/get_fields'; | ||
| const url = new URL(`https://${proxy ?? ''}${domain}${path}`); | ||
| url.searchParams.set('access_token', accessToken); | ||
| debug('getFields url:', url); |
There was a problem hiding this comment.
If the debug function is made asynchronous (as suggested for src/log.ts to avoid blocking I/O), this call should be awaited to ensure proper execution and error handling.
| debug('getFields url:', url); | |
| await debug('getFields url:', url); |
| debug('getFields url:', url); | ||
| const res = await fetch(url); | ||
| const data = await res.json(); | ||
| debug('getFields response:', data); |
There was a problem hiding this comment.
If the debug function is made asynchronous (as suggested for src/log.ts to avoid blocking I/O), this call should be awaited to ensure proper execution and error handling.
await debug('getFields response:', data);| TEXT = 1, // 文本 | ||
| SELECT = 2, // 选项 | ||
| TIME = 3, // 时间 | ||
| IMAGE = 4, // 图片 | ||
| FILE_SINGLE = 5, // 单个文件 | ||
| FILE_MULTI = 6, // 多个文件 |
There was a problem hiding this comment.
The comments for the FieldType enum values are in Chinese. For consistency with the rest of the codebase which primarily uses English for comments and identifiers, it would be better to translate these comments to English.
TEXT = 1, // Text
SELECT = 2, // Select
TIME = 3, // Time
IMAGE = 4, // Image
FILE_SINGLE = 5, // Single file
FILE_MULTI = 6, // Multiple files| export const debug = (...args: unknown[]) => { | ||
| if (isVerbose()) { | ||
| console.log(...args); | ||
|
|
||
| Deno.writeTextFileSync( | ||
| resolve(import.meta.dirname!, '../temp/debug.json'), | ||
| JSON.stringify(args, null, 2), | ||
| ); | ||
| } | ||
| }; |
There was a problem hiding this comment.
The debug function uses Deno.writeTextFileSync, which is a synchronous and blocking I/O operation. If debug is called frequently, this could impact performance. Consider using the asynchronous version Deno.writeTextFile to avoid blocking the event loop. This will require making the debug function async.
| export const debug = (...args: unknown[]) => { | |
| if (isVerbose()) { | |
| console.log(...args); | |
| Deno.writeTextFileSync( | |
| resolve(import.meta.dirname!, '../temp/debug.json'), | |
| JSON.stringify(args, null, 2), | |
| ); | |
| } | |
| }; | |
| export const debug = async (...args: unknown[]) => { | |
| if (isVerbose()) { | |
| console.log(...args); | |
| await Deno.writeTextFile( | |
| resolve(import.meta.dirname!, '../temp/debug.json'), | |
| JSON.stringify(args, null, 2), | |
| ); | |
| } | |
| }; |
1 participant
close #15
close #14