-
Notifications
You must be signed in to change notification settings - Fork 1
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
Imgee V2 #59
Imgee V2 #59
Changes from 22 commits
441629f
7337d16
ba53794
412e44b
9a49863
127da69
be447ac
3b54fbd
6cd46ce
57a99f0
643e476
9ab5a45
d57fec0
e0f547f
cb7b9bd
c6c4645
b3ff9f4
c663086
e4ae277
eb5cab0
b4a14c1
6fabf71
fc28fea
edc1bf9
95c1615
95adecf
4451fb0
4635676
838d1ec
418e080
f112e6c
cc369e6
380bb87
f2c6fd6
23007ec
59bb581
d31b027
ff390de
6731ef9
3dcb9e5
d2c1228
88457f5
102e603
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,6 +14,10 @@ | |
import os.path | ||
import sys | ||
from glob import glob | ||
from alembic import op | ||
from sqlalchemy.sql import select | ||
from sqlalchemy.orm.session import sessionmaker | ||
from sqlalchemy.orm import load_only | ||
|
||
sys.path.append('../../') | ||
from imgee import db | ||
|
@@ -22,7 +26,10 @@ | |
|
||
|
||
def upgrade(): | ||
imgs = StoredFile.query.filter_by(size=None) | ||
connection = op.get_bind() | ||
Session = sessionmaker(bind=connection.engine) | ||
session = Session(bind=connection) | ||
imgs = session.query(StoredFile).filter_by(size=None).options(load_only("id", "name", "title")) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i believe Alembic gives you a session already. Is a new session necessary? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jace can do that with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's a problem. Migrations must never import models. They must always be locally defined in the migration. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The reason is that the model may no longer exist in the same form in the code when the migration is run. You need version controlling. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jace that's exactly why I did this. The way it was before, the model was imported and used which was breaking the migrations. The way above, it uses the session of the migration, so it uses the version of the model at that point in migration and not the current version of the model. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That doesn't make sense. You're importing from models. You always get the current version of the model. |
||
|
||
for img in imgs: | ||
path = path_for(img.name) + '.*' | ||
|
@@ -34,7 +41,7 @@ def upgrade(): | |
print 'updated attributes of %s\n' % img.title, | ||
else: | ||
print 'local file not found for %s\n' % img.title, | ||
db.session.commit() | ||
session.commit() | ||
|
||
|
||
def downgrade(): | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,7 +3,6 @@ | |
# The imports in this file are order-sensitive | ||
|
||
import os | ||
from celery import Celery | ||
|
||
from flask import Flask, redirect, url_for | ||
from flask.ext.lastuser import Lastuser | ||
|
@@ -12,20 +11,18 @@ | |
import coaster.app | ||
from ._version import __version__ | ||
|
||
|
||
version = Version(__version__) | ||
app = Flask(__name__, instance_relative_config=True) | ||
lastuser = Lastuser() | ||
celery = Celery() | ||
|
||
assets['imgee.css'][version] = 'css/app.css' | ||
|
||
from . import models, views | ||
from .models import db | ||
from .api import api | ||
from .async import TaskRegistry | ||
from .tasks import TaskRegistry | ||
|
||
registry = TaskRegistry(os.getenv('ENV', 'production')) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any dependency on system environment should be in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can I set this inside the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hardcode |
||
|
||
registry = TaskRegistry() | ||
|
||
def mkdir_p(dirname): | ||
if not os.path.exists(dirname): | ||
|
@@ -40,14 +37,12 @@ def error403(error): | |
def init_for(env): | ||
coaster.app.init_app(app, env) | ||
baseframe.init_app(app, requires=['baseframe', 'picturefill', 'imgee']) | ||
app.error_handlers[403] = error403 | ||
app.error_handlers[403] = error403 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this Flask's recommended way of defining an error handler? If not, we should use the recommended way. |
||
lastuser.init_app(app) | ||
lastuser.init_usermanager(UserManager(db, models.User)) | ||
if app.config.get('MEDIA_DOMAIN') and ( | ||
app.config['MEDIA_DOMAIN'].startswith('http:') or | ||
app.config['MEDIA_DOMAIN'].startswith('https:')): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. According to the docs, |
||
app.config['MEDIA_DOMAIN'] = app.config['MEDIA_DOMAIN'].split(':', 1)[1] | ||
mkdir_p(app.config['UPLOADED_FILES_DEST']) | ||
celery.conf.add_defaults(app.config) | ||
registry.set_connection() | ||
app.register_blueprint(api, url_prefix='/api/1') |
This file was deleted.
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,7 @@ | ||
# -*- coding: utf-8 -*- | ||
|
||
from coaster.sqlalchemy import BaseNameMixin, BaseScopedNameMixin | ||
import imgee | ||
from imgee import app, url_for | ||
from imgee.models import db | ||
from imgee.utils import newid, guess_extension | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use dotted syntax as far as possible: from .. import app, url_for
from . import db
from ..utils import newid, guess_extension Also, what is this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there any advantage of using the relative import over absolute one? I usually feel a little confused whenever I face relative import. If the module name is present instead, it's clear on first glance where it's being imported from. And yeah, that's the same from flask. |
||
|
||
|
@@ -58,7 +58,29 @@ def __repr__(self): | |
def extn(self): | ||
return guess_extension(self.mimetype, self.orig_extn) or '' | ||
|
||
def is_queued_for_deletion(self): | ||
if imgee.app.config.get('CELERY_ALWAYS_EAGER'): | ||
return False | ||
return imgee.registry.is_queued_for_deletion(self.name+self.extn) | ||
def dict_data(self): | ||
return dict( | ||
title=self.title, | ||
uploaded=self.created_at.strftime('%B %d, %Y'), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use |
||
filesize=app.jinja_env.filters['filesizeformat'](self.size), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not just call this function directly? Unless it's an inbuilt Jinja filter? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's an inbuilt filter of jinja. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like it needs a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we want the binary format? By default it's |
||
imgsize='%s x %s' % (self.width, self.height), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
url=url_for('view_image', profile=self.profile.name, image=self.name), | ||
thumb_url=url_for('get_image', image=self.name, size=app.config.get('THUMBNAIL_SIZE')) | ||
) | ||
|
||
def add_labels(self, labels): | ||
new_labels = set(labels) | ||
old_labels = set(self.labels) | ||
if new_labels != old_labels: | ||
self.labels = labels | ||
|
||
if (new_labels == old_labels): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Brackets aren't really necessary here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed them. |
||
status, diff = '0', [] | ||
elif (new_labels > old_labels): | ||
status, diff = '+', (new_labels - old_labels) | ||
elif (old_labels > new_labels): | ||
status, diff = '-', (old_labels - new_labels) | ||
else: | ||
status, diff = '', new_labels | ||
|
||
return status, list(diff) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What if you had both additions and deletions? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This part of the code is a bit hairy. I think it can be handled better. I'll try something and show you. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No change here? |
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.
What goes in this folder?
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.
Uploads made during running the tests.