chore(gae): delete regions from migration/ndb/overview/main.py #13078
Conversation
There was a problem hiding this comment.
Hi team,
I'm currently reviewing this pull request and will provide my feedback shortly. In the meantime, here's a summary of the changes for you and other reviewers to quickly get up to speed:
This pull request, submitted by @eapl-gemugami, aims to delete references to specific App Engine regions from the migration/ndb/overview sample. The intent is to simplify the sample and make it more generic, addressing internal issue b/390488769.
Here's a breakdown of the changes:
appengine/standard/migration/ndb/overview/README.md: Updated deployment instructions to reflect only Python 3.8 support, removing outdated instructions for Python 2.7 and 3.7. Specifically, lines 14-19 were removed.appengine/standard/migration/ndb/overview/main.py: Removed several comment blocks related to region-specific configurations. These blocks were removed from lines 15, 28, 36, 51, 61, and 84.appengine/standard/migration/ndb/overview/requirements-test.txt: Addedsix==1.17.0and updatedpytestversion for Python 3 compatibility. Lines 3-6 were added.appengine/standard/migration/ndb/overview/templates/index.html: A minor fix correcting a trailing space in line 46.
The author has confirmed that tests and lint checks passed. They've also followed the sample guidelines and updated the README.
And now, a little haiku to lighten the mood:
Code flows like a stream,
Cleaning up, a smoother path,
Review's gentle hand.
|
Here is the summary of changes. You are about to delete 4 region tags.This comment is generated by snippet-bot.
|
There was a problem hiding this comment.
This pull request removes outdated regions and updates the Python version to 3.8. The changes are well-organized and easy to follow. The updated README accurately reflects the new deployment instructions. The addition of six==1.17.0 to requirements-test.txt is a good practice for ensuring compatibility across different Python 3 versions. I appreciate the thoroughness of the checklist, confirming that tests and lint checks pass.
| gcloud app deploy | ||
|
|
||
| To deploy and run this sample in App Engine standard for Python 3.7: | ||
| To deploy and run this sample in App Engine standard for Python 3.8: |
There was a problem hiding this comment.
Since the code now targets Python 3.8, the README should specify Python 3.8 or later. Per the Authoring Guide, supporting a range of Python versions is preferred.
| To deploy and run this sample in App Engine standard for Python 3.8: | |
| To deploy and run this sample in App Engine standard for Python 3.8 or later: |
| @@ -43,4 +43,4 @@ | |||
| <input type="submit" value="switch" /> | |||
| </form> | |||
| </body> | |||
| </html > | |||
| </html> | |||
There was a problem hiding this comment.
The HTML file is missing a doctype declaration. Adding <!DOCTYPE html> as the first line will help ensure that the page renders consistently across different browsers. Reference commonly used style guides, such as the W3C HTML validator.
<!DOCTYPE html>
<html>* chore(gae): delete regions from migration/ndb/overview/main.py * chore(gae): add support for Python 3 to requirements-test.txt * chore(gae): fix whitespace in templates/index.html * docs(gae): update README.md to a current Python 3 sample
Description
Fixes Internal: b/390488769
Checklist
nox -s py-3.12(see Test Environment Setup)nox -s lint(see Test Environment Setup)