-
Notifications
You must be signed in to change notification settings - Fork 236
Reorganize source code #12
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
Thank you for the PR @olibre. I will review this asap :) |
|
I think @olibre made some nice changes (e.g. using pipenv). @cosmic-byte would you be updating https://www.freecodecamp.org/news/structuring-a-flask-restplus-web-service-for-production-builds-c2ec676de563/ to reflect any new changes? that article has some really good SEO and I personally have been finding it really helpful. |
matthewliuswims
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.
I just have a couple of small comments. I just started tinkering around with python web development - so IDK enough to approve. But looks like it's great work👍
| To run application: make run | ||
|
|
||
| To run all commands at once : make all | ||
| Motivation |
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.
potentially move this Motivation section from the README and put into the PR comment or commit description? I don't think new-timers would want this context explanation (since they'd have none). Same for "this is a fork..." and "the current example source code" - don't think you'd want these to be actually merged
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 I agree with all your remarks.
I will take time to improve/fix my PR in the coming week…
| import os | ||
|
|
||
| # uncomment the line below for postgres database url from environment variable | ||
| # uncomment the line below for postgres data url from environment variable |
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.
nit: why the change? since below it says os.environ['DATABASE_URL']
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.
Sorry this is a mistake.
I prefer to keep postgres database.
I do not know why I did that change. 😳
|
Thank you for your PR. Please if you can make a more granular pull request that improves what we have and does not defeat the purpose of this boilerplate, it would be much appreciated. |
|
Sorry I do not use Flask since Oct. 2019, I have switched to FastAPI and now, I am currently working on other programming languages. Thank you for maintaining your awesome open source repository 👍 |
Lot's of changes, but original spirit is kept 😄
/user->/usersrequirements.txtbyPipfile(pip->pipenv)README.md