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

Add 'expand' flag to the web plugin #2050

Merged
merged 7 commits into from Jun 26, 2016

Conversation

Projects
None yet
4 participants
@maxammann
Contributor

maxammann commented Jun 13, 2016

Hello,

I'm currently developing an android app to synchronize your beets library with your android phone. This addition helps me to implement the query and simply some stuff.

There are some screenshots of my app attached.

screenshot_20160613-140950
screenshot_20160613-140956

@@ -55,7 +55,7 @@ def _rep(obj, expand=False):
return out
def json_generator(items, root):
def json_generator(items, root, expand=False):

This comment has been minimized.

@jackwilsdon

jackwilsdon Jun 13, 2016

Member

Do you think you could document the expand argument in the docstring too?

@@ -310,6 +314,9 @@ def func(lib, opts, args):
self.config['port'] = int(args.pop(0))
app.config['lib'] = lib
# Normalizes json output
app.config['JSONIFY_PRETTYPRINT_REGULAR'] = False

This comment has been minimized.

@jackwilsdon

jackwilsdon Jun 13, 2016

Member

Is there a need for this in production? From the Flask docs:

If this is set to True (the default) jsonify responses will be pretty printed if they are not requested by an XMLHttpRequest object (controlled by the X-Requested-With header).

This comment has been minimized.

@maxammann

maxammann Jun 13, 2016

Contributor

After experiencing with the json output of jsonify and json.dumps I noticed that the output of json.dumps is always compact while jsonify outputs it prettified.

That's the first problem, the second is that a prettified json string takes mus more space. Even the compat string of my library is already > 30 MB.

This comment has been minimized.

@jackwilsdon

jackwilsdon Jun 13, 2016

Member

Oh, oops! I read it as True instead of False, haha. Sorry about that and I agree with your reasoning 100%.

@jackwilsdon

This comment has been minimized.

Member

jackwilsdon commented Jun 13, 2016

Looks like our flake8 test is failing due to some lines being more than 79 characters in length, but apart from that (and the comments I previously made) this looks great! Thanks! 🎉

yield ']}'
def is_expand(args):

This comment has been minimized.

@jackwilsdon

jackwilsdon Jun 13, 2016

Member

Do we really need this method? It might be simpler to just do this:

return flask.jsonify(_rep(entities[0], expand=flask.request.args.get('expand')))

This comment has been minimized.

@maxammann

maxammann Jun 13, 2016

Contributor

Not sure, if I remove the method I would need to use the "in" operator

return flask.jsonify(_rep(entities[0], expand='expand' in flask.request.args))

This would also cause duplicate code. What do you think?

This comment has been minimized.

@jackwilsdon

jackwilsdon Jun 13, 2016

Member

Hmm, we'd be better with @sampsyo's opinion on this I think.

Here's how I'd (personally) write the method:

def is_expand():
    """Returns whether the current request is for an expanded response."""

    return flask.request.args.get('expand') is not None

We can directly access flask.request.args without passing it in, as it's a property of the flask module.

@maxammann

This comment has been minimized.

Contributor

maxammann commented Jun 13, 2016

Thanks, added the recommendations

@maxammann

This comment has been minimized.

Contributor

maxammann commented Jun 13, 2016

Is there a reson for line 40: del out['path']

The path variable would be very useful as I do not know the extension of the individual files and I'm not sure how the "format" tag is defined.

Thanks in advance!

@jackwilsdon

This comment has been minimized.

Member

jackwilsdon commented Jun 13, 2016

Hmm, looks like that was added by @sampsyo in 8493909. Not entirely sure why it's there, maybe we'd be better not giving the "full" path but instead the path relative to the beets library?

@maxammann

This comment has been minimized.

Contributor

maxammann commented Jun 13, 2016

Oh yes, didn't notice that it returns the full path. How about replacing it with obj.destination(fragment=True) instead of deleting it?

EDIT: Going to open a new pull request soon

@sampsyo

This comment has been minimized.

Member

sampsyo commented Jun 13, 2016

Cool! This looks like a great project. Is it open-source also?

In the future, I'd love to talk to you about basing this kind of thing on the new AURA API we're developing: https://github.com/beetbox/aura

This API, when we've got it implemented, will include native support for stuff like this via "includes," which will let clients request exactly what data they want in their response. It would be great to collaborate on this if you're interested.

@maxammann

This comment has been minimized.

Contributor

maxammann commented Jun 13, 2016

Yeah of course! FOSS FTW! :P
https://github.com/maxammann/music-cyclon (Still very bare repo whichi is being bootstrapped atm)

Started the rewrite a few days ago when I realized I could just integrate it directly into beets 😄
I also wanted to follow the material design guide lines.
Also a better api would be gold for music-cyclon as the unspecific requests are very slow and take a lot of time.

The synchronize part is the first milestone. I'm not sure yet whether streaming will be a thing or maybe remotly managing the library. For now synchronizing with poweramp integration is enough for me. Let me know what you think!

Looking forward to talk per chat or maybe even VOIP.

@jackwilsdon

This comment has been minimized.

Member

jackwilsdon commented Jun 13, 2016

You could join the beets IRC channel on freenode 😄. You can join here: freenode webchat.

@jackwilsdon

This comment has been minimized.

Member

jackwilsdon commented Jun 13, 2016

Wow @sampsyo, I can't believe I've never seen aura! It looks very fleshed out and well designed 😄. What's the long term plan for beets and aura? Create an aura plugin for beets?

@sampsyo

This comment has been minimized.

Member

sampsyo commented Jun 13, 2016

Yeah, that's the plan! The hope is to start with a new beets plugin that fits the API and then work on other parts of the ecosystem.

@@ -197,6 +197,7 @@ The interface and response format is similar to the item API, except replacing
the encapsulation key ``"items"`` with ``"albums"`` when requesting ``/album/``
or ``/album/5,7``. In addition we can request the cover art of an album with
``GET /album/5/art``.
You can also add the '?expand' flag to get the individual items of an album.

This comment has been minimized.

@jrobeson

jrobeson Jun 18, 2016

Contributor

can you provide an example of the usage of ?expand here?

@sampsyo

This comment has been minimized.

Member

sampsyo commented Jun 18, 2016

This looks ready to merge as soon as we have a changelog entry.

@maxammann

This comment has been minimized.

Contributor

maxammann commented Jun 26, 2016

@sampsyo
Web plugin Changelog:

  • Added an option to show the items of an album
  • Added the 'path' tag to the json outpu of a file which shows the relative path to the file
  • Normalized the json output
@sampsyo

This comment has been minimized.

Member

sampsyo commented Jun 26, 2016

Could you please add that to changelog.rst? Then somebody can hit that green button.

@maxammann maxammann force-pushed the maxammann:master branch from dd73cfd to cfd70c2 Jun 26, 2016

@maxammann

This comment has been minimized.

Contributor

maxammann commented Jun 26, 2016

Updated fork and added changelog

@sampsyo

This comment has been minimized.

Member

sampsyo commented Jun 26, 2016

Awesome; thank you!

@sampsyo sampsyo merged commit 77a869d into beetbox:master Jun 26, 2016

1 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@maxammann

This comment has been minimized.

Contributor

maxammann commented Jun 26, 2016

Thanks as well :P
Do you know when the next release is going to be published?

@sampsyo

This comment has been minimized.

Member

sampsyo commented Jun 26, 2016

We don't have a precise release schedule, usually, but please feel free to weigh in if you think a faster release would ever be useful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment