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

Automatic formatting for Album & Item #1269

Merged
merged 8 commits into from
Jan 26, 2015

Conversation

brunal
Copy link
Contributor

@brunal brunal commented Jan 25, 2015

Cut the need to format manually (and often incorrectly) when logging by implementing the __format__ magic method.

In beets.ui,

  • delete _pick_format
  • update print_obj
  • improve print_
  • add format_ (like format but the format spec can be None)

Next steps are:

  • adapt print & logging in plugins: if there's no real intent for a special formatting, just use the template configured (future commits in this PR)
  • make -p (path) and -f (format) top-level options that can be used in all situations (future PR, raises option name conflicts problems)
  • rename options list_format_{item,album} to format_{item,album}? (can be this PR, + we should keep the current names for retro-compatibility in any case)

This started by a discussion on PR #1262.

Cut the need to format manually (and often incorrectly) when logging by
implementing the __format__ magic method (see PEP 3101) on LibModel
(the parent class of Album & Item).

Based on a discussion in PR beetbox#1262
Code now relies on `format()` for items and albums displaying/logging.
`ui.print_()` calls `unicode()` or `str()` on the strings so for most
usages calling `ui.print_(obj)` replaces `ui.print_(obj, lib, None)`.

Where there is a special format `ui.print_(format(obj, fmt))` is fine,
but when `fmt` can be None then one has to call
`ui.print_(ui.format_(obj, fmt))` -- which is what `ui.print_obj` now
does.
if not spec:
spec = beets.config[self._format_config_key].get(unicode)
result = self.evaluate_template(spec)
if isinstance(spec, bytes):
Copy link
Member

Choose a reason for hiding this comment

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

Is this necessary? I would have guessed that evaluate_template would always return a Unicode—is that incorrect?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I get it—you're supporting formatting with both bytes and Unicode templates. Sorry for being slow. Perhaps a comment to this effect would make it clearer?

@sampsyo
Copy link
Member

sampsyo commented Jan 26, 2015

Looks great. This simplifies a lot of logging and listing statements, which is awesome to have. I have a few comments inline.

I conceptually like making -p and -f top-level flags, but for some reason that seems a little less intuitive to me from a CLI UI standpoint. Typing beet ls -p seems so much more natural to me than typing beet -p ls. I'm not sure what the right solution is, but I would love to find something that both avoids duplication and yet still feels intuitive.

@sampsyo
Copy link
Member

sampsyo commented Jan 26, 2015

Modulo these last two tiny behavioral tweaks (do we need format_, and how should print_ handle non-strings) this looks ready. We can tackle the other work (possibly renaming config options, and tracking down any other spuriously specific formatting) after merging.

Delete `ui.format_` and then `ui.print_obj`. Simply ensure that when
there is no format it defaults to '' = default format = config option.
Since this raises problems the best is probably to maintain the base
behaviour: expect byte strings or unicodes.
@brunal
Copy link
Contributor Author

brunal commented Jan 26, 2015

Since print_() expecting strings has been working fine so far the best if probably to keep it that way: I removed the added check+cast in print_() and adapted a couple of print_(item).

@sampsyo
Copy link
Member

sampsyo commented Jan 26, 2015

Awesome. I'll merge this now & write a changelog entry.

@sampsyo sampsyo merged commit 4aba432 into beetbox:master Jan 26, 2015
sampsyo added a commit that referenced this pull request Jan 26, 2015
Automatic formatting for Album & Item
sampsyo added a commit that referenced this pull request Jan 26, 2015
@sampsyo
Copy link
Member

sampsyo commented Jan 26, 2015

Merged up! Thanks again for taking care of this. ✨ It's something I've been hoping to clean up for a long time.

This and #1271 might also make the PR in #1092 more palatable in the short term. Woohoo!

@brunal brunal deleted the libmodels-formatting branch January 27, 2015 06:58
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.

2 participants