search

thedataincubator / forecaster

3 stars 1 forks source link

Change index page to landing page #5

Closed ZachGlassman closed 6 years ago

ZachGlassman commented 6 years ago

Addresses https://github.com/thedataincubator/forecaster/issues/6

craw-daddy commented 6 years ago

Ok, so my understanding here is that the html filename would have to be changed from "index.html" to "predict.html" for this change to actually be effective. (My understanding of Flask is growing rather slowly.)

Or is it that the function name for the route needs to be changed? So

@app.route('/predict') def predict(): ...

ZachGlassman commented 6 years ago

The template rendered is controlled by the render_template function, the route itself does not necessarily need to be related

craw-daddy commented 6 years ago

Sorry, maybe I edited my comment after you saw it... (Bad practice...)

Does the name of the function that defines the route need to match the route?

@app.route('/predict') def predict(): ...

dzbee commented 6 years ago

Nope! @app.route('/whatever') simply says that the URL/endpoint will be [domain]/whatever (e.g. www.example.com/whatever). The function name and the template name used in render_template() are free be whatever you like, though it is conventional that these three things match.

craw-daddy commented 6 years ago

Ok, then I guess I don't understand what is causing the tests to fail in this case. Any hint?

ZachGlassman commented 6 years ago

The hint is that my commit on this branch caused the tests to fail. The tests check that there is something at /.

craw-daddy commented 6 years ago

Ok, well given that, I'd tend to reject this change since the tests are failing. (I thought that the idea of accepting a change is that the result will be "deployable" after the change is accepted.)

ZachGlassman commented 6 years ago

Valid point in general, however I would like you to write code to make the tests pass. Sort of like test driven development.

craw-daddy commented 6 years ago

Ok, let's approve this and I'll work on issue #6 to create an index page.

ZachGlassman commented 6 years ago

please do so on this branch and when tests pass we will review.

ZachGlassman commented 6 years ago

this looks good, tests are failing because they are testing the / route for predictions but its no longer the predict route, so you will need to change them to hit the /predict route.

craw-daddy commented 6 years ago

Ok, I am pretty certain that I'm not understanding everything that's going on here with the tests. ;)

ZachGlassman commented 6 years ago

The tests are mostly passing, you will need to fix a few things for pylint and then this is good!

forecaster/forecaster.py:15:0: E0011: Unrecognized file option 'diasable' (unrecognized-inline-option) forecaster/forecaster.py:15:4: W0612: Unused variable 'landing' (unused-variable)

so I think the first one is just a typo (diasable != disable) and the second one might also disappear if that first one is fixed.

craw-daddy commented 6 years ago

Checks have passed now. The merge conflict needs to be resolved. But I think this resolves this issue, as well as Issue 6.

dzbee commented 6 years ago

I'll let @ZachGlassman voice his own opinion here, but usually the way I address conflicts is to locally rebase the branch on master (resolving the conflicts locally during the rebase), close the current PR and delete the remote branch, and then push the local branch to the remote and open a new PR.

ZachGlassman commented 6 years ago

Please do as Dylan suggested and assign me to merge.