#1 Changes in order to make the application run
Closed 6 years ago by abompard. Opened 6 years ago by atelic.
atelic/plus-plus-service master  into  master

file modified
+1
@@ -3,3 +3,4 @@ 

  *.egg-info

  *.db

  .tox

+ *.pyc 

Ah, right, that's been in my global gitignore file since forever :-)

\ No newline at end of file

file modified
+17 -21
@@ -1,30 +1,25 @@ 

  import os

  import flask

  

- from .karma import KarmaManager, NoSuchFASUser

- 

- 

- APP = flask.Flask(__name__)

- APP.config.from_object('{}.defaults'.format(__name__))

- if 'FLASK_SETTINGS' in os.environ:

-     APP.config.from_envvar('FLASK_SETTINGS')

+ from karma import KarmaManager, NoSuchFASUser

+ from app import APP

I think that the relative import issues you're getting come from the fact that you're trying to run the app as a script. The Flask docs now recommend another way, see the very beginning of http://flask.pocoo.org/docs/0.11/quickstart/
Running "FLASK_APP=plus_plus_service flask run" works fine for me with the current code.

  

  

  def check_auth():

-     # Do something like https://pagure.io/pagure/blob/master/f/pagure/api/__init__.py#_79

+     # TODO something like:

+     # https://pagure.io/pagure/blob/master/f/pagure/api/__init__.py#_79

      pass

  

- 

  #

  # Database

  #

  

+ 

  @APP.before_request

  def db_connect():

-     from .database import Database

-     flask.g.db = Database(

-         APP.name, APP.config["SQLALCHEMY_DATABASE_URI"]

-         ).get_session()

+     from database import Database

+     flask.g.db = Database(APP.name,

+                           APP.config["SQLALCHEMY_DATABASE_URI"]).get_session()

That is styling, I guess? PEP8 authorizes both ways. (at least flake8 does not report the current one).

  

  

  @APP.teardown_appcontext
@@ -32,18 +27,19 @@ 

      if hasattr(flask.g, "db"):

          flask.g.db.remove()

  

- 

  #

  # Views

  #

  

- @APP.route('/user/<username>')

+ 

+ @APP.route('/user/<username>/', methods=['GET', 'POST'])

+ @APP.route('/user/<username>', methods=['GET', 'POST'])

Ah, right, thanks, I did not test the POST yet, it would have failed.

  def view_user(username):

-     kmgr = KarmaManager(flask.g.db)

+     karma_manager = KarmaManager(flask.g.db)

Agreed 100%, naming is important.

  

      if flask.request.method == 'GET':

          # Get the user's stats

-         stats = kmgr.stats(username)

+         stats = karma_manager.stats(username)

          stats.update(dict(username=username))

          return flask.jsonify(stats)

  
@@ -54,14 +50,14 @@ 

          increment = ('decrement' in flask.request.form and

                       bool(flask.request.form['decrement']))

          try:

-             current_karma = kmgr.change(

-                 flask.g.fas_user, username, increment),

+             current_karma = karma_manager.change(flask.g.fas_user, username,

+                                                  increment),

          except NoSuchFASUser as error:

              return str(error), 404

          return flask.jsonify(username=username, karma=current_karma)

  

-     else:

-         flask.abort(400)  # Bad Request

  

+ if __name__ == '__main__':

+     APP.run()

  

  # vim:set shiftwidth=4 tabstop=4 expandtab textwidth=79:

@@ -0,0 +1,8 @@ 

+ import os

+ 

+ import flask

+ 

+ APP = flask.Flask(__name__)

Hmm, if you move this in its own "app" module, then you'll have __name__ == plus_plus_service.app, right ?

+ APP.config.from_pyfile('defaults.py')

+ if 'FLASK_SETTINGS' in os.environ:

+     APP.config.from_envvar('FLASK_SETTINGS')

@@ -1,9 +1,11 @@ 

+ from sqlalchemy import create_engine, MetaData

+ from sqlalchemy.orm import scoped_session, sessionmaker

What's the rationale for this change? I thought PEP8 recommended imports to be sorted alphabetically?

+ 

  from alembic import command as alembic_command

  from alembic.config import Config as AlembicConfig

  from alembic.migration import MigrationContext

  from alembic.script import ScriptDirectory

- from sqlalchemy import create_engine, MetaData

- from sqlalchemy.orm import scoped_session, sessionmaker

+ 

  

  

  class Database(object):
@@ -26,7 +28,7 @@ 

      def get_session(self):

          session = scoped_session(sessionmaker(

              autocommit=False, autoflush=False, bind=self.engine))

-         from .models import Base

+         from models import Base

          Base.query = session.query_property()

          return session

  
@@ -44,7 +46,7 @@ 

          # import all modules here that might define models so that

          # they will be registered properly on the metadata.  Otherwise

          # you will have to import them first before calling init_db()

-         from .models import Base

+         from models import Base

          Base.metadata.create_all(bind=self.engine)

          alembic_command.stamp(self.alembic_cfg, "head")

  

file modified
+9 -9
@@ -4,8 +4,8 @@ 

  from sqlalchemy.orm.exc import NoResultFound

  from sqlalchemy.sql.expression import func

  

- from .lib import make_fas_client, get_current_release, load_fedmsg_config

- from .models import Vote

+ from lib import make_fas_client, get_current_release, load_fedmsg_config

+ from models import Vote

  

  import logging

  logger = logging.getLogger(__name__)
@@ -29,16 +29,16 @@ 

              req_params={'username': username})

          return bool(request['success'])

  

-     def change(self, agent, recip, increment=True):

+     def change(self, agent, recipient, increment=True):

Agreed again, I used the code from Supybot but full names are much better.

          # Send the karma change to fedmsg.

          # Store the change in the database.

          # Return the new karma value.

  

          # Check that the recipient is a FAS user

-         if not self._username_exists(recip):

+         if not self._username_exists(recipient):

              logger.info(

-                 "Saw %s from %s, but %s not in FAS", recip, agent, recip)

-             raise NoSuchFASUser("Couldn't find %s in FAS." % recip)

+                 "Saw %s from %s, but %s not in FAS", recipient, agent, recipient)

+             raise NoSuchFASUser("Couldn't find %s in FAS." % recipient)

  

          release = get_current_release()

          vote = 1 if increment else -1
@@ -46,7 +46,7 @@ 

          # Check our karma db to make sure this hasn't already been done.

          try:

              existing = self.db.query(Vote).filter_by(

-                 release=release, from_username=agent, to_user=recip).one()

+                 release=release, from_username=agent, to_user=recipient).one()

          except NoResultFound:

              pass

          else:
@@ -56,7 +56,7 @@ 

              return

  

          base_query = self.db.query(

-             func.sum(Vote.value)).filter_by(to_username=recip)

+             func.sum(Vote.value)).filter_by(to_username=recipient)

          total_all_time = base_query.scalar()

          total_this_release = base_query.filter_by(release=release).scalar()

  
@@ -65,7 +65,7 @@ 

              modname="irc", topic="karma",  # TODO: fix modname

              msg={

                  'agent': agent,

-                 'recipient': recip,

+                 'recipient': recipient,

                  'total': total_all_time,  # The badge rules use this value

                  'total_this_release': total_this_release,

                  'vote': vote,

file modified
+1 -1
@@ -9,7 +9,7 @@ 

      # Initialize FAS client

      # To get the information, we need a username and password to FAS.

      # DO NOT COMMIT YOUR USERNAME AND PASSWORD TO THE PUBLIC REPOSITORY!

-     from . import APP

+     from app import APP

      return AccountSystem(

          APP.config["FAS_URL"],

          username=APP.config["FAS_USERNAME"],

@@ -3,8 +3,8 @@ 

  

  # add your model's MetaData object here

  # for 'autogenerate' support

- from plus_plus_service import APP

- from plus_plus_service.models import Base

+ from app import APP

+ from models import Base

  url = APP.config["SQLALCHEMY_DATABASE_URI"]

  target_metadata = Base.metadata

  

file modified
+2 -2
@@ -1,5 +1,5 @@ 

- from . import APP

- from .database import Database

+ from app import APP

+ from database import Database

  

  

  def sync_db():

Had to make some changes so that the app would run including:

  • remove all relative imports (found via ag -i '(from\ +\.\ *(\w+))|(import\ +\.\ *(\w+))') since they were causing errors
  • add the methods argument to @APP.route so that POST will work, flask will also return a 405 if it receives a request not in the list so remove the last if statement.
  • Call APP.run so I can make requests to the API

Also adds flake8 and style fixes.

Ah, right, that's been in my global gitignore file since forever :-)

I think that the relative import issues you're getting come from the fact that you're trying to run the app as a script. The Flask docs now recommend another way, see the very beginning of http://flask.pocoo.org/docs/0.11/quickstart/
Running "FLASK_APP=plus_plus_service flask run" works fine for me with the current code.

That is styling, I guess? PEP8 authorizes both ways. (at least flake8 does not report the current one).

Ah, right, thanks, I did not test the POST yet, it would have failed.

Agreed 100%, naming is important.

What's the rationale for this change? I thought PEP8 recommended imports to be sorted alphabetically?

Agreed again, I used the code from Supybot but full names are much better.

Since a lot of changes in this PR come from removing the relative imports, which actually came from a lack of documentation on my side (on how to run it), if you don't mind I'll just cherry-pick the other changes.

I realize that I pushed the code to give you an idea of its state but that it was still a bit early for other people to dig into it. Thanks for your input, I'll add more docs :-)

Hmm, if you move this in its own "app" module, then you'll have __name__ == plus_plus_service.app, right ?

Interesting, I had never heard of the flask run command. In that case you can cherry pick the changes you wish and I will close this. Thanks for the info! :)

Yeah, Pingou was surprised too, but it's apparently the recommended way to run it. I don't know if it's recent or not.

I've cherry-piked the changes, thanks for the MR.

Pull-Request has been closed by abompard

6 years ago