search

ethanheyrman / restaurant-roulette

0 stars 1 forks source link

Shane commit with CSV file #29

Closed supple2 closed 4 years ago

BijanT commented 4 years ago

This is good with me if you want to merge it, @ethanheyrman

BijanT commented 4 years ago

I looks like you pushed up a lot of pycache files. Could you remove them and add "__pycache__/" to the gitignore?

BijanT commented 4 years ago

@ethanheyrman can you remind me what the reason was for removing "migrations/" from the gitignore was? I'm bringing it up because migration files were added in this commit. I'm thinking they shouldn't be included because they're auto generated by python manage.py makemigrations

ethanheyrman commented 4 years ago

@BijanT no, you are correct. I think we touched on this briefly in our last face2face, but it definitely slipped through the cracks. The only reason I can advocate for not including the migrations in a .gitignore is if we want to squash migrations or in some other way manually modify the files in a way that needs to be reflected for every developer. I think for the scope of this project its definitely not necessary to edit them, as environment setup and app launch are both pretty straightforward processes. i'm totally on board for .gitignoring them

BijanT commented 4 years ago

It looks like that fix did the trick. Now all you should need to do is:

  1. Remove the remaining pychache and migration files
  2. add migrations/ to the gitignore
  3. add requests==2.18.4 to api_restaurant_roulette/requirements/base.txt

Let me know if you have any questions.

ethanheyrman commented 4 years ago

@supple2 and @BijanT, do you both feel this is in a place that's ready to merge?

BijanT commented 4 years ago

The main code works fine. requests==2.18.4 hasn't been added to the requirements file yet, but I can add that to the PR I'm going to write updating the README to use the script Shane wrote. So, I'm fine with this being merged