search

Freeseer / freeseer

Designed for capturing presentations at conferences. Pre-fill a list of talks to record, record them, and upload them to YouTube with our YouTube Uploader.
http://freeseer.readthedocs.org
GNU General Public License v3.0
215 stars 110 forks source link

Refactor and decouple REST api framework #569

Closed dbrenden closed 9 years ago

dbrenden commented 9 years ago

The recording API endpoints have been moved from the server.py module to a new class called recording.py.

dideler commented 9 years ago

Great, can you just move the reference to 568 from the title to the PR description? Please put it at the bottom of the description as "Closes #568".

Edit: It's common to use imperative style when using Git. So "Moved recording ..." would be "Move recording ...". There's lots of resources online explaining why this is preferred, if you're interested in learning more.

zxiiro commented 9 years ago

Regarding the adding "Closes #568" to the description. You should actually add it to your commit message so that it is linked in the commit message back to the issue.

Your commit message should look something like

Moved recording API endpoints from server.py to recording.py
Some detailed description if neccessary
Closes #568
dideler commented 9 years ago

You should actually add it to your commit

Yeah, since these commits are squashed that would work (I don't always squash my commits, which is why I rather close it from the PR). Having the reference in both places (i.e. PR description and commit message) would be most convenient, and is simple enough to do.

mtomwing commented 9 years ago

FYI, the build failed because of some flake8 errors:

@RUNNING: flake8 src
src/freeseer/frontend/controller/__init__.py:3:1: F401 'recording' imported but unused
src/freeseer/frontend/controller/recording.py:200:1: W391 blank line at end of file
src/freeseer/frontend/controller/server.py:90:27: F821 undefined name 'ServerError'
@EXIT WITH STATUS: 1
@FAILING...
mtomwing commented 9 years ago

Looks good. Though there is still room for more refactoring. Like for example since all the endpoints return JSON, it would be nice if there was a decorator that did the transformation automatically and then the endpoint function could simply return a dict.

mtomwing commented 9 years ago

On IRC you mentioned that you fixed the last few things I mentioned, but I don't see any changes.

mtomwing commented 9 years ago

Assuming this is already rebased against master, please squash it and I'll do the merge later today.

dideler commented 9 years ago

@dbrenden Michael's afk right now, but he told me over Hangouts that he wants you to squash it into a single commit since it's all refactoring work.

mtomwing commented 9 years ago

Once the commit message is fixed up I'll merge this. As mentioned on IRC I want a description of what happened, not a list of every single change.

mtomwing commented 9 years ago

Thanks for your contribution!

dideler commented 9 years ago

Looking at the comments, this was originally supposed to close #568 but it never did. @dbrenden what's the status of #568?

dbrenden commented 9 years ago

The work for #568 is done so it should be closed.

On 21 Oct 2014 07:43, "Dennis Ideler" notifications@github.com wrote:

Looking at the comments, this was originally supposed to close #568 but it never did. @dbrenden what's the status of #568?

— Reply to this email directly or view it on GitHub.