fix: consistent formatting and numerous lint errors #600
Conversation
jpoehnelt
changed the title
Summary of ChangesHello @jpoehnelt, 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 integrates BiomeJS into the project's workflow for Google Apps Script files, aiming to standardize code style and improve maintainability. The changes encompass the addition of a dedicated script to manage the formatting process and a broad application of these new style rules across the existing codebase. This initiative ensures a more consistent and cleaner code style moving forward. 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 introduces a wide range of formatting and linting fixes across numerous files, significantly improving code consistency and adhering to modern JavaScript standards. The changes include converting var to let/const, using arrow functions, and template literals, which is great. I've also noticed fixes for critical issues like duplicate function declarations. I've identified a couple of areas where the changes might have introduced unintended side effects or could be made more robust. My comments below highlight these points for your consideration.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a script for running Biome on .gs files and applies extensive formatting and linting fixes across the entire codebase. The changes improve code consistency and adhere to modern JavaScript standards. My review focuses on ensuring the new script is robust and that the refactoring hasn't introduced any regressions, particularly around error handling. I've identified a few critical issues where error handling was removed and a high-severity issue with an unused variable in advanced/docs.gs. Additionally, I've found some opportunities to improve the new biome-gs.ts script for better performance and robustness.
| function readFirstParagraph(documentId) { | ||
| try { | ||
| // Get the document using document ID | ||
| const document = Docs.Documents.get(documentId, {'includeTabsContent': true}); | ||
| const firstTab = document.tabs[0]; | ||
| const bodyElements = firstTab.documentTab.body.content; | ||
| for (let i = 0; i < bodyElements.length; i++) { | ||
| const structuralElement = bodyElements[i]; | ||
| // Print the first paragraph text present in document | ||
| if (structuralElement.paragraph) { | ||
| const paragraphElements = structuralElement.paragraph.elements; | ||
| let paragraphText = ''; | ||
| // Get the document using document ID | ||
| const document = Docs.Documents.get(documentId, { | ||
| includeTabsContent: true, | ||
| }); | ||
| const firstTab = document.tabs[0]; | ||
| const bodyElements = firstTab.documentTab.body.content; | ||
| for (let i = 0; i < bodyElements.length; i++) { | ||
| const structuralElement = bodyElements[i]; | ||
| // Print the first paragraph text present in document | ||
| if (structuralElement.paragraph) { | ||
| const paragraphElements = structuralElement.paragraph.elements; | ||
| let paragraphText = ""; | ||
|
|
||
| for (let j = 0; j < paragraphElements.length; j++) { | ||
| const paragraphElement = paragraphElements[j]; | ||
| if (paragraphElement.textRun !== null) { | ||
| paragraphText += paragraphElement.textRun.content; | ||
| } | ||
| for (let j = 0; j < paragraphElements.length; j++) { | ||
| const paragraphElement = paragraphElements[j]; | ||
| if (paragraphElement.textRun !== null) { | ||
| paragraphText += paragraphElement.textRun.content; | ||
| } | ||
| console.log(paragraphText); | ||
| return paragraphText; | ||
| } | ||
| console.log(paragraphText); | ||
| return paragraphText; | ||
| } | ||
| } catch (e) { | ||
| // TODO (developer) - Handle exception | ||
| console.log('Failed with error %s', e.message); | ||
| } | ||
| } |
There was a problem hiding this comment.
The try...catch block around the Docs.Documents.get call has been removed. This removes necessary error handling. If the API call fails (e.g., document not found, permissions error), the script will crash. The error handling should be restored to ensure the script is robust and provides meaningful error messages.
function readFirstParagraph(documentId) {
try {
// Get the document using document ID
const document = Docs.Documents.get(documentId, {
includeTabsContent: true,
});
const firstTab = document.tabs[0];
const bodyElements = firstTab.documentTab.body.content;
for (let i = 0; i < bodyElements.length; i++) {
const structuralElement = bodyElements[i];
// Print the first paragraph text present in document
if (structuralElement.paragraph) {
const paragraphElements = structuralElement.paragraph.elements;
let paragraphText = "";
for (let j = 0; j < paragraphElements.length; j++) {
const paragraphElement = paragraphElements[j];
if (paragraphElement.textRun !== null) {
paragraphText += paragraphElement.textRun.content;
}
}
console.log(paragraphText);
return paragraphText;
}
}
} catch (e) {
// TODO (developer) - Handle exception
console.log('Failed with error %s', e.message);
}
}| async function findGsFiles( | ||
| dir: string, | ||
| fileList: string[] = [], | ||
| ): Promise<string[]> { | ||
| const files = readdirSync(dir); | ||
| for (const file of files) { | ||
| const filePath = join(dir, file); | ||
| if ( | ||
| file === "node_modules" || | ||
| file === ".git" || | ||
| file === "dist" || | ||
| file === "target" || | ||
| file === "pkg" | ||
| ) { | ||
| continue; | ||
| } | ||
| const stat = statSync(filePath); | ||
| if (stat.isDirectory()) { | ||
| await findGsFiles(filePath, fileList); | ||
| } else if (file.endsWith(".gs") && file !== "moment.gs") { | ||
| fileList.push(filePath); | ||
| } | ||
| } | ||
| return fileList; | ||
| } |
There was a problem hiding this comment.
The findGsFiles function uses synchronous file system operations (readdirSync, statSync) within an async function. This mix of synchronous and asynchronous code is inefficient and can block the Node.js event loop, which can be slow for large repositories.
A better approach is to use the promise-based APIs from node:fs/promises and Promise.all to perform directory traversal concurrently. This will significantly improve performance and make the code more robust.
Please also update your imports from node:fs to use readdir from node:fs/promises and remove the now-unused statSync.
async function findGsFiles(dir: string): Promise<string[]> {
const ignoredDirs = new Set([
"node_modules",
".git",
"dist",
"target",
"pkg",
]);
try {
const dirents = await readdir(dir, { withFileTypes: true });
const files = await Promise.all(
dirents.map(async (dirent) => {
const fullPath = resolve(dir, dirent.name);
if (ignoredDirs.has(dirent.name)) {
return [];
}
if (dirent.isDirectory()) {
return findGsFiles(fullPath);
}
if (dirent.isFile() && dirent.name.endsWith(".gs") && dirent.name !== "moment.gs") {
return [fullPath];
}
return [];
}),
);
return files.flat();
} catch (error) {
// Ignore errors for directories that can't be read (e.g. permissions)
console.error(`Could not read directory ${dir}:`, error);
return [];
}
}1 participant
No description provided.