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

AURA: Add aura plugin and docs #3758

Merged
merged 15 commits into from
Mar 8, 2021
Merged

AURA: Add aura plugin and docs #3758

merged 15 commits into from
Mar 8, 2021

Conversation

govynnus
Copy link
Member

This is a server implementation of an AURA server.
tox -e lint fails (see here), which would be easy enough to fix but puzzles me because no other plugins with the same "problems" cause an error. Does anyone know what's up with that?

@govynnus
Copy link
Member Author

It turns out that a lot of files (including most plugins) are ignored by flake8 in setup.cfg.

@martinkirch
Copy link

martinkirch commented Nov 21, 2020

Hello,
I just gave it a try ; it works better than expected ! Even with a URI prefix. It also seems to allow searching tracks by flexible attribute (I tested my usual beets filter mix=party), but maybe it's just luck ?

I'd suggest a slight precision in the documentation about the reverse proxy : when proxying with Apache, one have to put a trailing slash to the app URL.

Thanks for this plug-in !

@govynnus
Copy link
Member Author

@martinkirch That's good to hear!
Thanks for the feedback on the documentation, I'll update it now.

Not just luck, it should work for all beets fields including flexible ones :-)
AURA says:

Servers may also provide other, non-standard fields not listed in this specification.

Copy link
Member

@sampsyo sampsyo left a comment

Choose a reason for hiding this comment

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

With profuse apologies for taking so long to get around to reviewing this, I finally got a chance to read through with a fine-toothed comb. This is incredibly great work and I'm super excited. The implementation seems solid, of course, but I also think this is a super useful exercise to have done to "debug" the AURA spec. Much of the stuff under "Issues" in the docs are actually not the fault of the plugin at all but should trigger reflection in the design of AURA.

I only have a few very tiny nitpicks here and there. I would love to merge this if you're still up for it!

beetsplug/aura.py Outdated Show resolved Hide resolved
beetsplug/aura.py Outdated Show resolved Hide resolved
beetsplug/aura.py Outdated Show resolved Hide resolved
beetsplug/aura.py Outdated Show resolved Hide resolved
beetsplug/aura.py Outdated Show resolved Hide resolved
beetsplug/aura.py Outdated Show resolved Hide resolved
beetsplug/aura.py Outdated Show resolved Hide resolved
beetsplug/aura.py Outdated Show resolved Hide resolved
beetsplug/aura.py Outdated Show resolved Hide resolved
docs/plugins/aura.rst Show resolved Hide resolved
@govynnus
Copy link
Member Author

govynnus commented Mar 6, 2021

No need to apologize! Thank you for looking through in so much detail 😄
Definitely still interested in merging (still need to set a server version and do a changelog entry though).

Still working on the tests....

Seems like there are some linting errors since dc0350d. Should I rebase or something to get rid of those, or is it not a problem?

P.S. Apologies if the name change was confusing :-)

@sampsyo
Copy link
Member

sampsyo commented Mar 6, 2021

Agh; those linter errors! This is one of those cases where the style checker gets updated and reclassifies some old code as an error. I believe we resolved this through configuration recently—any chance you could merge or rebase to make sure this branch also has that updated configuration (which should eliminate those spurious errors)?

And yay, I'm glad you're still interested! 😃 🚀

Not confused by the name change yet. At least your avatar is still the same pleasant circle. 😆

displayable_path may remove 'bad' characters, yielding a wrong path.

Also use track.path rather than track.destination() as that is where
the file is actually located rather than where it should be located
according to the beets path system.
Ensure aura branch has updated configuration to eliminate spurious
linter errors from the style checker.
govynnus added a commit to beetbox/aura that referenced this pull request Mar 7, 2021
@govynnus
Copy link
Member Author

govynnus commented Mar 7, 2021

Ok, I've set the server version, added a changelog entry, and the linter errors are no more 😄

Still no tests, but they will probably be delayed (again) since I have exams coming up soon :(

@sampsyo
Copy link
Member

sampsyo commented Mar 7, 2021

Well, that's awesome news! How would you feel about merging this even before there are tests? It seems worthwhile enough to have it in-tree—then we can come back to testing in a separate PR. (Feel free to hit the green button yourself if you agree. 😃)

@govynnus govynnus merged commit debd382 into master Mar 8, 2021
@govynnus govynnus deleted the aura branch March 8, 2021 07:25
@govynnus
Copy link
Member Author

govynnus commented Mar 8, 2021

Done 🎉
Thanks for all your help on this!

@sampsyo
Copy link
Member

sampsyo commented Mar 8, 2021

Wahoo! I am positively thrilled. Thanks for the careful work, both on this plugin and on the spec!! ✨ 🚀

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.

3 participants