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

Upload zip to server #64

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

Conversation

tscritch
Copy link
Contributor

@tscritch tscritch commented May 4, 2018

Change Reader object to store all of the data instead of returning it from the compile functions
Change app routes to use reader values
Add upload view to upload a zip file
Some tech debt:

  • if the --no-browser is set then the app will go to the upload view. for the CLI we want it to go directly to the /channels url
  • the archives directory needs to be wiped on the server every time a new zip is uploaded

# TODO: Make sure this works
with io.open(os.path.join(self._PATH, "users.json"), encoding="utf8") as f:
self.__USER_DATA = {u["id"]: u for u in json.load(f)}
def __init__(self, PATH=''):
Copy link
Owner

Choose a reason for hiding this comment

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

nit: PATH should be path

flash("No selected file")
return redirect(request.url)
if file and allowed_file(file.filename):
print("made it and it should extract the archive here")
Copy link
Owner

Choose a reason for hiding this comment

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

I guess there isn't great logging set up already, so print is fine, but at the same time, I want to avoid having "made it" etc. logged :P
Perhaps "Extracting archive..." is sufficient.

file = request.files["archive_file"]
# if user does not select file, browser also
# submit a empty part without filename
if file.filename == "":
Copy link
Owner

Choose a reason for hiding this comment

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

if not file.filename works too

@@ -7,14 +10,19 @@
static_folder="static"
)

reader = Reader()

# these functions only fire when the route is navigated to
Copy link
Owner

Choose a reason for hiding this comment

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

Do you need this comment?

@@ -72,9 +86,46 @@ def mpim_name(name):
mpim_users=mpim_users)


@app.route("/")
ALLOWED_EXTENSIONS = set(["zip"])
Copy link
Owner

Choose a reason for hiding this comment

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

Constants like this should be at the top of the file

def allowed_file(filename):
return "." in filename and \
filename.rsplit(".", 1)[1].lower() in ALLOWED_EXTENSIONS

Copy link
Owner

Choose a reason for hiding this comment

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

Two newlines between all module-level functions


@app.route("/", methods=["GET", "POST"])
def upload():
print('upload')
Copy link
Owner

Choose a reason for hiding this comment

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

I guess the request I have general for these print statements is, can you just clean them up a bit?

flex-direction: column;
justify-content: center;
align-items: center;
background-color: #f5f5f5
Copy link
Owner

Choose a reason for hiding this comment

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

The mismatched indentation in this file bothers me, please use an auto formatter of some kind in your editor to clean it up. Thanks. :)

@@ -0,0 +1,70 @@
@import url('https://fonts.googleapis.com/css?family=Lato:400,900');
Copy link
Owner

Choose a reason for hiding this comment

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

Same for the indentation in this file

@tscritch
Copy link
Contributor Author

tscritch commented May 4, 2018

Hey @hfaran just mostly wanted to show you what I worked on. I had to get this out this week for my work. I can fix all of this stuff.
What do you think about the implementation?

@hfaran
Copy link
Owner

hfaran commented May 4, 2018

Hey @hfaran just mostly wanted to show you what I worked on. I had to get this out this week for my work. I can fix all of this stuff.
What do you think about the implementation?

Well, as long as:

  • it works
  • is mostly bug-free
  • you resolve all of the comments I had

implementation looks fine to me. 👍 😄

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.

2 participants