- Notifications
You must be signed in to change notification settings - Fork6.3k
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Address issue#1
ThiefMaster left a comment
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 runpycodestyle (successor of thepep8 cli) to check if it reports any issues.
HelloFlask/views.py Outdated
| fromflaskimportFlask,render_template | ||
| fromHelloFlaskimportapp |
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 forhello_flask sincesnake_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
| returnrender_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
| 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
| fromflaskimportFlask | ||
| fromflaskimportrender_template | ||
| fromdatetimeimportdatetime | ||
| fromreimportmatch |
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 justimport re and then usere.match() - this also avoids shadowing when people assignmatch = ... 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
| returnapp.send_static_file('data.json') | ||
| returnapp.send_static_file('data.json') | ||
| defclean_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
| defclean_name(name): | ||
| # 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'ther 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.
| <spanclass="message">{{message }}</span> It's {{ date }}. | ||
| <spanclass="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 youcat 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. :(
kraigb commentedJul 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. |
hello_app/views.py Outdated
| @app.route('/') | ||
| defhome(): | ||
| returnrender_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 thetitle="..." since that's something you'd usually put in your template.
Therender_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.
kraigb commentedJul 11, 2018
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
…t-labFix 101-devtest-labs automation test by adding some default value for variables
trymicrosoft#2 [skip ci]
Address issue#1