-
Notifications
You must be signed in to change notification settings - Fork 2k
chore(ci): migrate datastore/functions to new ci #4080
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
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 @glasnt, 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 migrates the datastore/functions sample to a new CI setup. This involves several changes to modernize the code and align it with current standards. Specifically, it updates the import syntax to use ES modules, modifies the function export syntax, and adjusts the CI configuration files to reflect these changes.
Highlights
- CI Migration: The primary goal is to migrate the datastore/functions sample to a new CI pipeline.
- ES Module Syntax: The code is updated to use ES module
importsyntax instead ofrequire. - Function Exports: The function export syntax is updated to use
export async functioninstead ofexports.functionName = async. - Configuration Files: The CI configuration files are updated to reflect the new setup, including adding the module to the
nodejs-dev.jsoncand removing it fromnodejs.jsonc.
Changelog
Click here to see the changelog
- .github/config/nodejs-dev.jsonc
- Added 'datastore/functions' to the list of modules to be tested in the development CI.
- .github/config/nodejs.jsonc
- Removed 'datastore/functions' from the list of modules to be tested in the main CI, likely because it's now handled by the development CI or has been updated to resolve previous errors.
- datastore/functions/index.js
- Updated import syntax from
requireto ES moduleimportfor@google-cloud/datastore(line 17). - Modified function export syntax from
exports.functionName = asynctoexport async functionforset(line 61),get(line 97), anddel(line 127) functions.
- Updated import syntax from
- datastore/functions/package.json
- Added
"type": "module"to the package.json file to enable ES module support (line 13).
- Added
- datastore/functions/test/index.test.js
- Updated import syntax from
requireto ES moduleimportfor various modules likeassert,child-process-promise,path,uuid,sinon,node-fetch,wait-port, and@google-cloud/datastore(lines 17-24).
- Updated import syntax from
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.
ES modules arise,
'Require' fades from our eyes,
CI's new embrace,
Modern code, clean space,
Datastore functions now wise.
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
The pull request migrates the datastore/functions sample to the new CI. This involves updating the configuration files and modifying the code to use ES modules. The changes seem reasonable, but I have a few suggestions to improve the code quality and maintainability.
Summary of Findings
- Inconsistent use of
requireandimport: The code uses bothrequireandimportstatements. It's better to be consistent and use onlyimportstatements since the project is now using ES modules. - Missing JSDoc comments for exported functions: The exported functions
set,get, anddelare missing JSDoc comments. Adding these comments will improve the code's documentation and make it easier to understand.
Merge Readiness
The pull request is almost ready to be merged. I recommend addressing the comments related to the use of require and import statements, and adding JSDoc comments for the exported functions. I am unable to directly approve the pull request, and other reviewers should review and approve this code before merging.
| 'use strict'; | ||
|
|
||
| const {Datastore} = require('@google-cloud/datastore'); | ||
| import {Datastore} from '@google-cloud/datastore'; |
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.
Consider using import for all dependencies to maintain consistency with ES module syntax. Since the package.json now specifies type: module, it's best to avoid require statements.
| import {Datastore} from '@google-cloud/datastore'; | |
| import { Datastore } from '@google-cloud/datastore'; |
| * @param {object} res Cloud Function response context. | ||
| */ | ||
| exports.set = async (req, res) => { | ||
| export async function set(req, res) { |
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.
Consider adding a JSDoc comment for the set function to describe its purpose, parameters, and return value. This will improve the code's documentation and make it easier to understand.
| export async function set(req, res) { | |
| /** | |
| * Creates and/or updates a record. | |
| * | |
| * @param {object} req Cloud Function request context. | |
| * @param {object} res Cloud Function response context. | |
| * @returns {Promise<void>} | |
| */ | |
| export async function set(req, res) { |
| * @param {object} res Cloud Function response context. | ||
| */ | ||
| exports.get = async (req, res) => { | ||
| export async function get(req, res) { |
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.
Consider adding a JSDoc comment for the get function to describe its purpose, parameters, and return value. This will improve the code's documentation and make it easier to understand.
/**
* Retrieves a record.
*
* @param {object} req Cloud Function request context.
* @param {object} res Cloud Function response context.
* @returns {Promise<void>}
*/
export async function get(req, res) {| * @param {object} res Cloud Function response context. | ||
| */ | ||
| exports.del = async (req, res) => { | ||
| export async function del(req, res) { |
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.
Consider adding a JSDoc comment for the del function to describe its purpose, parameters, and return value. This will improve the code's documentation and make it easier to understand.
/**
* Deletes a record.
*
* @param {object} req Cloud Function request context.
* @param {object} res Cloud Function response context.
* @returns {Promise<void>}
*/
export async function del(req, res) {| import {Datastore} from '@google-cloud/datastore'; | ||
|
|
||
| const datastore = new Datastore(); | ||
| const program = require('../'); |
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.
Description
Fixes #
Note: Before submitting a pull request, please open an issue for discussion if you are not associated with Google.
Checklist
npm test(see Testing)npm run lint(see Style)GoogleCloudPlatform/nodejs-docs-samples. Not a fork.