Skip to content
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

String replacement used in image-content-search Python example #679

Closed
samtwil opened this issue May 17, 2022 · 3 comments · Fixed by #1116
Closed

String replacement used in image-content-search Python example #679

samtwil opened this issue May 17, 2022 · 3 comments · Fixed by #1116
Labels
feature-request A feature should be added or improved. language/python Related to Python examples p1

Comments

@samtwil
Copy link
Contributor

samtwil commented May 17, 2022

Describe the bug

During code review of a separate PR, it was noticed that python/image-content-search/src/landingPage/main.py uses a string replacement. This is Not Great

Expected Behavior

No usage of the string.replace() method is expected

Current Behavior

Around line 14 of python/image-content-search/src/landingPage/main.py, we have the following line:

'body': file_get_contents("index.html").replace('###loginPage###', login_page)

Reproduction Steps

Open python/image-content-search/src/landingPage/main.py` and review the code, no deployment necessary

Possible Solution

One possible alternative is to use the sub function from the re module

import re
foo = 'this is my string'
rs.sub('my', '', foo) 
# foo = 'this is string'

Additional Information/Context

No response

CDK CLI Version

Framework Version

No response

Node.js Version

OS

Language

Python

Language Version

No response

@samtwil samtwil added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels May 17, 2022
@NGL321 NGL321 added feature-request A feature should be added or improved. language/python Related to Python examples p1 and removed bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Jun 3, 2022
@NGL321
Copy link
Contributor

NGL321 commented Jun 3, 2022

Since this is a request for an enhancement (the code isn't broken, just has a poor approach), I have switched the issue to a feature-request. However I have kept a high priority since it seems that this is something that should be tackled sooner than later.

Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

1 similar comment
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

kaiz-io added a commit that referenced this issue Dec 27, 2024
* Fix(py/image-content-search: Swap replace with format
Fixes #679

* Fix(py/appsync): Update code and testing deploying
Fixes #287

---------

Co-authored-by: Michael Kaiser <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request A feature should be added or improved. language/python Related to Python examples p1
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants