beetbox / beets

music library manager and MusicBrainz tagger
http://beets.io/
MIT License
12.74k stars 1.82k forks source link

Quieter deprecation warnings, at least for `beet completion` #1414

Open 45054 opened 9 years ago

45054 commented 9 years ago

https://bbs.archlinux.org/viewtopic.php?id=195893

The warning should only be displayed when beets itself is started. As of now with every start of bash the warning message is shown without any hint where it originates from.

sampsyo commented 9 years ago

That's very strange—we actually already do this:

The warning should only be displayed when beets itself is started.

The message is printed when beets starts. It must be the case, somehow, that beets was starting every time bash started. Do you have any clues as to why that would be the case?

45054 commented 9 years ago

Apparently, this problem only occures, when the bash-completion package is installed. Maybe it is an Arch bug, as the feature was introduced via this commit: https://projects.archlinux.org/svntogit/community.git/commit/trunk?h=packages/beets&id=c0da9b9e41da2424948173d485621175d0ef4083

sampsyo commented 9 years ago

Ah, I see—yes, that's running beet completion every time the shell starts up. That's fine, I suppose, but it's not something I expected. Aside from perhaps leaking some output to the console like this, it also can take some time since spinning up the Python interpreter can be slow.

Instead of (or in addition to) asking the packagers not to install the completion script by default, we could make the beet completion command very quiet by default so that warnings are never printed. Or we could just turn off this warning by default—it's not particularly critical that people migrate!

dimsuz commented 9 years ago

I wanted to open an issue, but it seems that my problem belongs here too. I too come from Arch Linux where beet completion is starting on each shell login. But in my case beet's blb file is stored on an external drive which is not always mounted.

So after recent upgrade to version 1.3.11 I started seeing this:

error: database file /var/run/media/dima/Home/dima/data/musiclibrary.blb could not be opened

I guess in this case beets should simply not even try to open this file, or accept failure.

dimsuz commented 9 years ago

Hmm, actually it is a bit worse: without this file in sight I cannot even run diagnostic commands like beet --version or beet --help

It throws this error and immediately quits.

Should I create a separate issue?

sampsyo commented 9 years ago

OK, that's another argument for quieter output in commands that might be run at shell startup. (I'd still recommend that people dump the beet completion output to a file and source it, though, since regenerating it each time could be slow.)

And yes, please do open another issue—it does seem like a good idea to let commands like version and help work without opening the database. I'm not yet sure how we'll actually accomplish that, but it does seem like a good goal. :smiley:

dimsuz commented 9 years ago

Yeah, 'eval'-ing beet completion on shell start up seems like a bad idea. I think this is questionable decision made by whoever created an AUR (=unofficial) beets package for Arch Linux.

As for the second thing, created an issue #1415. Unforutnately I can't create a PR or assist otherwise, because I don't know Python (atm)

alucryd commented 9 years ago

Apologies for the noise, I got a complaint from someone who thought adding one line to his .bashrc was too much of a hassle :P Adding that one line in /etc/bash_completion.d seemed like the only way to have bash completion enabled by default while taking enabled plugins into account, I went with it since I didn't experience issues, but obviously there are some. Just reverted that change in our repos, sometimes laziness is just not right ;)

brunal commented 9 years ago

@alucryd another solution is sourcing "beet completion" upon a request for completion. I had that at some point but not anymore. This seems to work:

_load_beet_completion() { eval "$(beet completion)"; }
complete -F _load_beet_completion beet

So the first time the user enters "beet " the completion will be produced and replace _load_beet_completion as the complete function.

@sampsyo Maybe that in this case the warning shouldn't be displayed. This would simply be setting the warning behaviour in the completion function. I'll take care of it.

brunal commented 9 years ago

Well actually I'm not sure I can take care of it: warnings/not critical logs should be silenced when we want to print completion → in beets.ui.commands.print_completion(). However this is triggered way after we read the config, open the library & co.! I don't really see an easy way to solve that.

sampsyo commented 9 years ago

@brunal True, it is a bit of an enigma—we really want to do lots of setup before we even know which command we're invoking. I also don't see an easy way around it. But if someone does have an ingenious idea, maybe the same mechanism could work for #1415…

Thanks for checking in, @alucryd. It was definitely a nice idea to have completion work out of the box. :smiley:

alucryd commented 9 years ago

@brunal Thx for the tip, seems to work like a charm :) @sampsyo Just gonna run this by those people who had issues, if all goes well I'll push bash completion back in our repos.

sampsyo commented 9 years ago

:+1: