-
Notifications
You must be signed in to change notification settings - Fork 6k
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
Conversation
Address issue #1
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.
You might want to run pycodestyle
(successor of the pep8
cli) to check if it reports any issues.
HelloFlask/views.py
Outdated
from re import match | ||
|
||
from flask import Flask, render_template | ||
|
||
from HelloFlask import 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.
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)
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.
Yes, good point given that I made that piece a module and not just a folder name.
HelloFlask/views.py
Outdated
|
||
@app.route('/hello/<name>') | ||
def hello_there(name): | ||
from datetime import datetime | ||
now = datetime.now() | ||
|
||
return render_template( | ||
"hello_there.html", | ||
title ='Hello, Flask', |
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.
no space before =
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.
OK
HelloFlask/views.py
Outdated
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), |
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.
no spaces around =
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.
OK
HelloFlask/views.py
Outdated
message = "Hello there, " + name + "!", | ||
date = now.strftime("%A, %d %B, %Y at %X") | ||
name = clean_name(name), | ||
date = datetime.now() |
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.
no spaces around =
HelloFlask/views.py
Outdated
from flask import Flask | ||
from flask import render_template | ||
from datetime import datetime | ||
from re import match |
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.
usually people just import re
and then use re.match()
- this also avoids shadowing when people assign match = ...
to the actual match object
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.
Makes sense...I'm still unclear when it's good to import a module as a whole vs. importing the part you need. :)
HelloFlask/views.py
Outdated
return app.send_static_file('data.json') | ||
|
||
def clean_name(name): |
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 is not really needed, since any variable passed to jinja is considered unsafe by default so any HTML there will be escaped
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.
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.
HelloFlask/views.py
Outdated
# 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) |
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.
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)
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.
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> |
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.
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)
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.
"files.insertFinalNewline": true is the setting, but it's false by default. :(
@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. |
hello_app/views.py
Outdated
|
||
@app.route('/') | ||
def home(): | ||
return render_template("home.html", title="Home") |
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.
I'd move that to the template using another {% block %}
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.
I don't understand what you mean. Is there a default rendering for / that we'd take advantage of here?
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.
I just meant the title="..."
since that's something you'd usually put in your template.
The render_template
calls you need to keep
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.
Ah, I see. Thanks.
Added the content block and revised the templates. Let me know if there's anything else, and thanks for your help! |
AzureRM Terraform Provider Contribution Guide
Templates naming
…t-lab Fix 101-devtest-labs automation test by adding some default value for variables
try microsoft#2 [skip ci]
Address issue #1