Do not try to open music database for basic commands (--help, --version, etc)

[dimsuz]

Current version (1.3.11) tries to open database file even for purely informational commands.
In cases when database file resides on some external and unmounted drive, this makes it impossible to see output of beet --help or beet --version, because user gets an error instead:

error: database file /path/to/musiclibrary.blb could not be opened

Would be nice to have beets provide this info without probing for the DB file.

[flight16]

I also encountered this just yesterday. I was unable to see the help output because it couldn't find my db file.

[sampsyo]

Seems like a good idea. Currently, the Library object is created and passed to every possible command. I suppose we could somehow do this lazily instead. Any suggestions would be appreciated.

[brunal]

beets/ui/__init__.py:1024: lib.get_item(0) # Test database connection.

Maybe this should simply be removed? Then there should not be a problem.

[sampsyo]

I don't think that will quite work—many of the errors (e.g., file doesn't exist) will be raised just when opening the database connection, i.e., on creating the Library instance:

>>> import sqlite3
>>> sqlite3.connect('/this/path/does/not/exist')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
sqlite3.OperationalError: unable to open database file

It's also nice to have that probe there so that other setup problems can be caught up-front and distinguished from legitimate programming mistakes.

[brunal]

Proposition:

the beets world has a number of attributes (including a Library instance and a config and other stuff). Each plugin can register with each of those attributes (via a static attribute?) in return of which their corresponding attribute (e.g. self.library) will be set in time (not always the same time: plugin instantiation for the config and certain event listeners of the library)*.

Upon instantiation of all parts Beets will register those requests and know what to send. A non-requested component won't be computed.

The evoked registration system feel a bit Java-OOP to me. Introspection could be used to collect component requests (feasible since concerned components are mostly singletons and therefore function arguments that expect one of them ­­are named after the class: library for Library, config for Config).

* It would require rewriting core commands as plugins, which IMHO isn't difficult or unwarranted, for the same patterns repeat in beets/ui/commands.py.

[sampsyo]

Yeah, interesting direction. It would be nice to have some way to encapsulate the required state.

As a counterpoint, here's something I've been thinking about for a little while that would address the problem (and a couple of others) in a slightly different way. What if there were a beets.Context class, which would encapsulate everything that beets needs to do its work? For instance:

• ctx.config is a property that lazily constructs a Configuration by reading config files.
• ctx.library is another lazy-constructed property. The Library is created based on the configuration. You could of course still construct Library objects yourself, but this would be a convenient way to get the shared Library. (The Library would probably retain a reference to the context to get at the configuration rather than referencing a global variable.)
• The context could also play the role of a "plugin manager", encapsulating all the global state that has previously given us headaches in the existing beets.plugins module.
• The context object could also take care of miscellaneous configuration-derived values—for example, the maximum filename length or the set of replacement patterns—so these don't have to be recomputed so many times.
• It may also contain the global logger.

Command functions (including built-in commands) would then be passed a context object instead of a Library and a Configuration. (Plugins should probably also get a reference to the Context, so perhaps passing to the command function wouldn't be strictly necessary—this close to your idea, but with one layer of indirection to decouple the Context from the BeetsPlugin.) This would make it easy to extend what constitutes global state while still making it clear where values come from.

I think this is a pretty standard dependency-injection kind of technique. LLVM, for example, has an LLVMContext class that encapsulates all state that's not thread-safe.

Does that sound reasonable at all? It would be a big task, but it could make a lot of things easier.

[brunal]

This sounds really good.

A downside I can see it that such code

class MyPlugin(BeetsPlugin):
    def __init__(self, config):
        super(MyPlugin, self).__init__()
        self.register_listener('import_task_file', self.listener)

    def listener(self, session, task):
        # do stuff with session & task

class TestMyPlugin(TestCase):
    def test_listener(self):
        mp = MyPlugin()
        result = mp.listener(session=..., task=...)
        self.assertStuff(result)

Would turn into the following

class MyPlugin(BeetsPlugin):
    # parent constructor receives and stores the context
    def listener(self):
        session = self.context.session
        task = self.context.task
        # do stuff with session & task

class TestMyPlugin(TestCase):
    def test_listener(self):
        context = Context(session=..., task=...)
        mp = MyPlugin(context)
        result = mp.listener()
        self.assertStuff(result)

(1) plugin methods will have to always start by fetching the desired
element instead of simply receiving it and (2) testing doesn't look as nice
anymore since we're not explicitly passing the arguments on which the test
focuses/relies. Overall we cannot know which method need what by looking at
the signature.

The context could also play the role of a "plugin manager", encapsulating
all the global state that has previously given us headaches in the existing
beets.plugins module.

This means that a plugin would hold a context but the context would also
own all plugins. I am not sure that this is good design, though it would be
convenient (a plugin that use another plugin could ask for its
instantiation once that way).

I'm really not sure whether I'm clear — it's not really clear in my head. I
think that this would solve many problems but I'm afraid we might fall into
anti-patterns, such as Context being a god object, and circular
dependencies.

I feel like several important issues and consequent solution have been
raised and proposed since 1.3.11 publication (#1409 and all encoding & path
length issues, this and #1414, convert issues, date management). Maybe it
is time to define milestones and feature freeze the next release.

edit: seems like by replying by email I disabled markdown parsing.

[sampsyo]

Yes, it definitely is a danger that a Context would swallow everything up and make the code harder to follow (the God Object anti-pattern). In that particular example, though, I wouldn't pack the ImportTask and ImportSession into the Context object. In particular, Context would be used for data that was (mostly) immutable—the default Library, the configuration, etc.

So that plugin event would still get task as a parameter, since there is not just one ImportTask for a given beets "instance". And the session would also be passed explicitly, to keep the importer conceptually separate from the basic beets infrastructure (which would, in theory, keep allowing multiple import sessions in the same beets process).

I still don't quite know whether this is a good idea at all either, but it might look something like this:

Where the solid arrows are (required) references and the dotted arrow is the context's reference to a default Library (although multiple libraries could refer to the same instance). The idea would be to wrap up everything that's currently a global (e.g., the configuration) and make it a Context member—but not Context-ify anything that we're currently passing around explicitly. (With the exception of plugin commands, which would now be able to decide whether or not they want a Library.)

---

Anyway, you're right! This is an interesting discussion that could use its own space. I'll make a discussion ticket for sorting out the next few milestones.

[duailibe]

What if there were a beets.Context class, which would encapsulate everything that beets needs to do its work?

This is a really interesting concept and the Pyramid web-framework relies on that a lot (as the request object). Working with for quite some time, I have followed the path of having all my classes receive that object and it turned out to be hell-ish. So, basically the pattern I follow these days is have each component know which dependencies they need and you declare how to instantiate each one of them. For instance:

def includeme(config):
    def user_service(request):
        return UserService(other_service=request.other_service)
    config.add_request_method(user_service)

Everything is evaluated lazily as well, taking advantage of the venusian module.

Anyway, I believe the important principle here is, IMO: context can hold all components instances, but no component can use it directly. Instead, they receive the instance of the component it depends.

[sampsyo]

context can hold all components instances, but no component can use it directly. Instead, they receive the instance of the component it depends.

Interesting! Is the idea to limit access to components that the code doesn't explicitly need? Are there examples from the Pyramid I should look at to learn more?

[duailibe]

Is the idea to limit access to components that the code doesn't explicitly need?

Yes! This is just to follow the "explicit is better than implicit" Python mantra as well :-)

Are there examples from the Pyramid I should look at to learn more?

I'm not sure, I haven't looked at any myself. We use Pyramid a lot where I work, in different services, and these were just conclusions we got at after making a lot of errors.

I wrote a simple example of how this would work in a real-ish example using Pyramid: https://gist.github.com/duailibe/da7f0c92e5383d441bf0.