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

Add 'expand' flag to the web plugin #2050

Merged
merged 7 commits into from Jun 26, 2016
Merged

Conversation

maxammann
Copy link
Contributor

@maxammann 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):
Copy link
Member

Choose a reason for hiding this comment

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

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

@jackwilsdon
Copy link
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):
Copy link
Member

Choose a reason for hiding this comment

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

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')))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

Thanks, added the recommendations

@maxammann
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
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
Copy link
Contributor Author

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
Copy link
Member

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

@jackwilsdon
Copy link
Member

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
Copy link
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.

Copy link

Choose a reason for hiding this comment

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

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

@sampsyo
Copy link
Member

sampsyo commented Jun 18, 2016

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

@maxammann
Copy link
Contributor Author

@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
Copy link
Member

sampsyo commented Jun 26, 2016

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

@maxammann
Copy link
Contributor Author

Updated fork and added changelog

@sampsyo
Copy link
Member

sampsyo commented Jun 26, 2016

Awesome; thank you! ✨

@sampsyo sampsyo merged commit 77a869d into beetbox:master Jun 26, 2016
@maxammann
Copy link
Contributor Author

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

@sampsyo
Copy link
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants