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

beet fields: Print flexible attributes #1818

Merged
merged 2 commits into from Jan 28, 2016
Merged

beet fields: Print flexible attributes #1818

merged 2 commits into from Jan 28, 2016

Conversation

GuilhermeHideki
Copy link
Member

Tries to solve #544.

I'm not sure if this needs an flag, since isn't slow (at least to me), but would avoid one db access.
If needs, what could be a good single letter/long option?

  • -x for --flexattr/--flexible-attributes ?
  • -f is usually format, so maybe not
  • -a is usually album

Should I add 'flexible' in 'item/album attributes' ?

@@ -92,6 +92,15 @@ def _print_rows(names):
print_("Album fields:")
_print_rows(library.Album.all_keys())

def _print_sqlite(query):
[print_(' ' * 2 + dict(zip(row.keys(), row))['key']) for row in query]
Copy link
Member

Choose a reason for hiding this comment

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

As a style matter, an imperative loop probably shouldn't be a written as a list comprehension—a normal for loop will likely be more readable.

Also, since this function doesn't close over any state from the outer scope, it should probably be a top-level function (rather than nested).

@sampsyo
Copy link
Member

sampsyo commented Jan 17, 2016

Great! This seems perfect!

I don't think we need a flag—this doesn't indeed seem like it could possibly be slow. If people complain, we can reconsider. :trollface:

@GuilhermeHideki
Copy link
Member Author

Did the suggestions. it seems ok now?

@sampsyo
Copy link
Member

sampsyo commented Jan 19, 2016

Looks great; thank you!

Could you please add some code comments and an entry to the changelog? It might also be worth looking at the docs for the fields command to see if we should add a note there. Then this should be ready to merge.

@@ -92,6 +97,14 @@ def _print_rows(names):
print_("Album fields:")
_print_rows(library.Album.all_keys())

with lib.transaction() as tx:
print_("Item attributes:")
Copy link
Member

Choose a reason for hiding this comment

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

Actually, let's change the terminology here and add the word "flexible" for clarity.

@GuilhermeHideki
Copy link
Member Author

I added some code comments to help and the changelog entry.
Don't know if this needs to add something in:

Show the item and album metadata fields available for use in Queries and Path Formats. Includes any template fields provided by plugins.

@sampsyo sampsyo merged commit 4605016 into beetbox:master Jan 28, 2016
sampsyo added a commit that referenced this pull request Jan 28, 2016
sampsyo added a commit that referenced this pull request Jan 28, 2016
sampsyo added a commit that referenced this pull request Jan 28, 2016
This isn't really a generic utility, since it is hard-coded to print the "key"
field of the result. So let's pair it with the place it's used.
sampsyo added a commit that referenced this pull request Jan 28, 2016
@sampsyo
Copy link
Member

sampsyo commented Jan 28, 2016

Great; thanks! It's all merged up.

And thanks again for all your contributions lately. I'll send you an invitation to the @beetbox org, so feel free to commit changes directly. PRs are of course still cool if you'd like a code review.

@GuilhermeHideki GuilhermeHideki deleted the fields-print-flexattr branch January 28, 2016 23:42
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

2 participants