8000 Updates per Adrian by kraigb · Pull Request #2 · microsoft/python-sample-vscode-flask-tutorial · GitHub
[go: up one dir, main page]

Skip to content

Updates per Adrian #2

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

Merged
merged 5 commits into from
Jul 11, 2018
Merged

Updates per Adrian #2

merged 5 commits into from
Jul 11, 2018

Conversation

kraigb
Copy link
Contributor
@kraigb kraigb commented Jul 11, 2018

Address issue #1

Address issue #1
@kraigb kraigb mentioned this pull request Jul 11, 2018
Copy link
@ThiefMaster ThiefMaster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might want to run pycodestyle (successor of the pep8 cli) to check if it reports any issues.

from re import match

from flask import Flask, render_template

from HelloFlask import app

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would go for hello_flask since snake_case names are more common for most python packages (some old stdlib packages aside, but unfortunately many older parts of stdlib aren't the best example for naming anyway)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, good point given that I made that piece a module and not just a folder name.


@app.route('/hello/<name>')
def hello_there(name):
from datetime import datetime
now = datetime.now()

return render_template(
"hello_there.html",
title ='Hello, Flask',

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no space before =

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK

return render_template(
"hello_there.html",
title ='Hello, Flask',
message = "Hello there, " + name + "!",
date = now.strftime("%A, %d %B, %Y at %X")
name = clean_name(name),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no spaces around =

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK

message = "Hello there, " + name + "!",
date = now.strftime("%A, %d %B, %Y at %X")
name = clean_name(name),
date = datetime.now()

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no spaces around =

from flask import Flask
from flask import render_template
from datetime import datetime
from re import match

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

usually people just import re and then use re.match() - this also avoids shadowing when people assign match = ... to the actual match object

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense...I'm still unclear when it's good to import a module as a whole vs. importing the part you need. :)

< 10000 summary role="button" data-target="details-collapsible.summaryElement details-toggle.summaryTarget" data-action="click:details-collapsible#toggle click:details-toggle#toggle" data-aria-label-closed="Expand comment" data-aria-label-open="Collapse comment" aria-expanded="true" aria-label="Collapse comment" data-view-component="true" class="py-2 px-3 rounded-2 color-bg-subtle">
return app.send_static_file('data.json')

def clean_name(name):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not really needed, since any variable passed to jinja is considered unsafe by default so any HTML there will be escaped

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course...I had it there in an intermediate step of the tutorial when I was just generating a plain text response, so I can omit it from the final version. Thanks for the reminder.

# Filter the name argument to letters only using regular expressions. URL arguments
# can contain arbitrary text, so we restrict to safe characters only.
match_object = match("[a-zA-Z]+", name)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you decide to keep it, I would replace this whole function with the much more simple:

return re.sub(r'[^a-zA-Z]+', '', name) or 'Friend'

the r prefix for the string is not needed here, but I think it's good practice to always use raw strings for regexps (so you don't forget it when adding something with a backslash later)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll be omitting all that extra code anyway.

<title>{{ title }}</title>
</head>
<body>
<span class="message">{{ message }}</span> It's {{ date }}.
<span class="message">Hello there, {{ name }}!</span> It's {{ date.strftime("%A, %d %B, %Y at %X") }}.
</body>
</html>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

file doesn't have an ending linebreak. not pretty when you cat such a file in a terminal 😿

sounds like VS code actually has an option for this: microsoft/vscode#1666
dunno if it's enabled by default though (I hope it is)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"files.insertFinalNewline": true is the setting, but it's false by default. :(

@kraigb
Copy link
Contributor Author
kraigb commented Jul 11, 2018

@ThiefMaster, take one more look. Renaming the folder means the diffs are gone, but the code is short and should have all the changes you requested.


@app.route('/')
def home():
return render_template("home.html", title="Home")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd move that to the template using another {% block %}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand what you mean. Is there a default rendering for / that we'd take advantage of here?

9E12

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just meant the title="..." since that's something you'd usually put in your template.

The render_template calls you need to keep

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see. Thanks.

@kraigb
Copy link
Contributor Author
kraigb commented Jul 11, 2018

Added the content block and revised the templates. Let me know if there's anything else, and thanks for your help!

@kraigb kraigb merged commit 042a160 into master Jul 11, 2018
@kraigb kraigb deleted the kraigb-issue001 branch July 11, 2018 18:44
caioroscelly added a commit to caioroscelly/python-sample-vscode-flask-tutorial that referenced this pull request Jun 10, 2024
MridulMoitra pushed a commit to MridulMoitra/python-sample-vscode-flask-tutorial that referenced this pull request Jul 31, 2024
AzureRM Terraform Provider Contribution Guide
MridulMoitra pushed a commit to MridulMoitra/python-sample-vscode-flask-tutorial that referenced this pull request Jul 31, 2024
MridulMoitra pushed a commit to MridulMoitra/python-sample-vscode-flask-tutorial that referenced this pull request Jul 31, 2024
MridulMoitra pushed a commit to MridulMoitra/python-sample-vscode-flask-tutorial that referenced this pull request Jul 31, 2024
…t-lab

Fix 101-devtest-labs automation test by adding some default value for variables
AdamZ45 added a commit to AdamZ45/python-sample-vscode-flask-tutorial that referenced this pull request Apr 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants
0