-
Notifications
You must be signed in to change notification settings - Fork 2k
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
fix: (appengine) update region tag in appengine/websockets/app.js & appengine/websockets/views/index.pug #3956
base: main
Are you sure you want to change the base?
fix: (appengine) update region tag in appengine/websockets/app.js & appengine/websockets/views/index.pug #3956
Conversation
Here is the summary of changes. You are about to add 4 region tags.
This comment is generated by snippet-bot.
|
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.
Hi team,
I'm currently reviewing this pull request and will provide my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly understand the changes:
This pull request by @hivanalejandro updates region tags in several files within the nodejs-docs-samples
repository's appengine/websockets
directory. The intent is to migrate region tags from appengine_websockets_...
to gae_websockets_...
, addressing several issues (b/391399101, b/391403733, b/391405800, b/391406425).
Here's a breakdown of the changes:
appengine/websockets/app.js
: The region tagappengine_websockets_app
is updated togae_websockets_app
on lines 17 and 43.appengine/websockets/views/index.pug
: The region tagsappengine_websockets_index
andappengine_websockets_form
are updated togae_websockets_index
andgae_websockets_form
respectively, on lines 15, 27, 31, 39, and 58.appengine/websockets/views/index.pug
: The region tagappengine_websockets_js
is updated togae_websockets_js
on lines 45 and 61.
The author has indicated that all tests and lint checks have passed. The pull request also includes a comprehensive checklist to ensure adherence to contribution guidelines.
A little haiku to lighten the mood:
Tags shift, code flows free,
Bugs vanish in the night's calm,
Clean code, joy abounds.
Thanks!
Gemini
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.
This pull request updates region tags in several files within the appengine/websockets
sample. The changes are straightforward and correctly address the specified bugs. The commit messages clearly explain the changes, and the code follows the project's style guidelines. I've referenced the Samples Style Guide in this review.
Here's a summary of the style guide elements I've checked:
- Region tags: The new region tags are correctly placed and formatted.
- Comments: The existing comments are clear and concise.
- Consistency: The changes maintain consistency with the rest of the codebase.
Overall, the changes are well-done and improve the accuracy of the sample code. The checklist items from the pull request template should be reviewed before merging.
// [START gae_websockets_app] | ||
// [START appengine_websockets_app] |
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.
Per the Samples Style Guide, it's best to remove the old region tags when they are no longer used. Since gae_websockets_app
replaces appengine_websockets_app
, the latter can be removed.
// [START gae_websockets_app] | |
// [START appengine_websockets_app] | |
// [START gae_websockets_app] | |
const app = require('express')(); |
@@ -39,5 +40,6 @@ | |||
}); | |||
} | |||
// [END appengine_websockets_app] |
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
Migrate region "appengine_websockets_app" to "gae_websockets_app"
Fixes b/391399101
Migrate region "appengine_websockets_form" to "gae_websockets_form"
Fixes b/391403733
Migrate region "appengine_websockets_index" to "gae_websockets_index"
Fixes b/391405800
Migrate region "appengine_websockets_js" to "gae_websockets_js"
Fixes b/391406425
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.