Skip to content

Python Getting Started "Bookshelf" app#210

Merged
bshaffer merged 14 commits intomasterfrom
bookshelf
Dec 6, 2019
Merged

Python Getting Started "Bookshelf" app#210
bshaffer merged 14 commits intomasterfrom
bookshelf

Conversation

@bshaffer
Copy link
Copy Markdown
Contributor

No description provided.

@bshaffer bshaffer requested a review from engelke November 22, 2019 21:55
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Nov 22, 2019
@bshaffer bshaffer requested a review from ace-n December 2, 2019 20:52
Copy link
Copy Markdown
Contributor

@busunkim96 busunkim96 left a comment

Choose a reason for hiding this comment

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

Nit: There's a mix of single and double quote usage. It might be nice to open a new issue to add a code formatter to this repo as well.

@bshaffer
Copy link
Copy Markdown
Contributor Author

bshaffer commented Dec 6, 2019

@busunkim96 I have a code formatter running for all PRs, which can be run locally using nox -s lint. However, it's not catching things like the quotes consistency. I did change all double quotes to single quotes where applicable!

Copy link
Copy Markdown
Contributor

@ace-n ace-n left a comment

Choose a reason for hiding this comment

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

LGTM once nits are resolved.


return redirect(url_for('.view', book_id=book['id']))

return render_template('form.html', action='Add', book={})
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: should this be at the top, like so?

if request.method != 'POST':
  return render_template(...)

...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think it's more idiomatic for python as is, mostly because this is how the previous bookshelf was set up. So I'd prefer to leave it as-is


return redirect(url_for('.view', book_id=book['id']))

return render_template('form.html', action='Edit', book=book)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same comment as above - should this be at the top?

@bshaffer bshaffer merged commit 1c9dfd1 into master Dec 6, 2019
@bshaffer bshaffer deleted the bookshelf branch December 9, 2019 22:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants