Skip to content
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

Create Flask app #3

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open

Create Flask app #3

wants to merge 16 commits into from

Conversation

SolarLiner
Copy link

@SolarLiner SolarLiner commented Nov 27, 2018

Hi,
So I made a Flask version of the app, with no imagination and by taking the design from the Node.js app - so it's pretty much built one-to-one with it.

To run, get Python 3.6 (at least), install pip and pipenv and then run pipenv install, and then pipenv run start. The dotenv file is read by pipenv when executing scripts, so no need for anymore manual action there.

I didn't run the acceptance tests yet, as I'd need to get myself a ruby environment first.

@SolarLiner
Copy link
Author

Just ran the acceptance test:

$ ./acceptance_test.rb
Run options: --seed 47649

# Running:

...F

Finished in 0.028574s, 139.9887 runs/s, 139.9887 assertions/s.

  1) Failure:
TodoMVPAcceptance#test_the_starting_page_has_the_right_html [./acceptance_test.rb:25]:
--- expected
+++ actual
@@ -17,11 +17,10 @@
 
     <section class=\"items\">
       <h2>Todo list</h2>
-      <ul>
-      
-      </ul>
+      <ul></ul>
     </section>
   
+
 </body>
 </html>
 "


4 runs, 4 assertions, 1 failures, 0 errors, 0 skips

The error is purely whitespace based which is acceptable.

@park-brian
Copy link

This looks great! It would be nice to see a plain python wsgi app as well (here's an example: https://docs.python.org/3.7/library/wsgiref.html#examples)

@grahamlyons
Copy link

This is a really nice looking Flask app but I think it probably violates this constraint: https://github.com/gypsydave5/todo-mvp#no-frameworks. Flask is a framework I like but it is a framework.

As a personal preference, and @gypsydave5 may disagree with me here, I'd prefer not to see .editorconfig or .vscode/ content committed. There are 16 files added and those account for 25%, There's also no path /home/solarliner/.local/share/virtualenvs/test-flask-app-VP6fqcCP/bin/python on my machine.

@SolarLiner
Copy link
Author

Hm, you are right - although I've never seen Flask as a framework but a library. I'd agree though that my Django PR shouldn't get accepted then.

Sorry about the VS Code folder. Normally I add it to gitignore before adding workspace settings but here VS Code detects the virtualenv and modifies settings on its own (hence the path to the venv python - I completely forgot about that.

My (limited) knowledge of Git is failing me here though; would it be enough to just add the paths to gitignore to discard them when the PR merges?

@grahamlyons
Copy link

grahamlyons commented Nov 29, 2018

To paraphrase (https://www.programcreek.com/2011/09/what-is-the-difference-between-a-java-library-and-a-framework/): "A library is just a collection of code definitions. The reason behind is simply code reuse. In framework, all the control flow is already there, and there's a bunch of predefined white spots that you should fill out with your code." Flask handles routing, request parsing, response dispatching, template rendering and a bunch of other things - you slot your business logic into a few places within that.

To get rid of those files from Git:

git rm -r todos/flask/.editorconfig todos/flask/.vscode/
git commit

For files like this that you want to ignore in every project but are specific to your set up you can use a global gitignore file (this was pointed out to me by my colleague who got sick of me adding Vim swap files to every repo's .gitignore...): http://egorsmirnov.me/2015/05/04/global-gitignore-file.html

@gypsydave5
Copy link
Owner

Crikey - this is embarrassing... I wasn't watching my own damn project and I've missed all the PRs! Rookie mistake - very, very sorry @SolarLiner - and thanks @grahamlyons for picking up the slack!

I'm thinking about relaxing the 'no frameworks' requirement (and so merging this PR) for two reasons:

  1. more take up
  2. easier to compare implementations

That is to say that it'd be interesting to see 'framework' solutions next to 'library' solutions. What do you think @grahamlyons and @quii

@gypsydave5
Copy link
Owner

Before I do merge it (if I do) I'd like the acceptance test to pass - yes I know it's 'only' a whitespace issue but I've yet to find a reasonable way to handle it. It should be fixable by making a small adjustment to the template - would you be happy to do that @SolarLiner ?

@SolarLiner
Copy link
Author

Should be a length check away - I'll also remove the VS Code-related files and such at the same time.

Expect something sometime later tonight (European time)!

@SolarLiner
Copy link
Author

Alright here's some more commits - Please do test it because neither the Ruby nor Rust acceptance tests work (Ruby gems don't install properly even though I have all required system libraries), and Rust just seems to complete without doing anything.

Checking manually though, it appears I've resolved the whitespace issue.

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.

4 participants