Raising error on missing configuration file inclusion

[adamjakab]

I think it is better to know than ignoring it silently. I don't know how you feel about this but I am requesting this PR because I lost a good deal of time convinced that my configuration was read only to discover that I mistyped the path. We could also opt for a 'softer' warning...

[sampsyo]

It seems fine to add a warning along these lines! I think I'd lean toward just a warning, however, rather than exiting with a traceback. Also, note that we already handle NotFoundError—maybe we should just print the message there instead of adding a new handler?

[adamjakab]

Ok for the warning (no traceback) part. However, I think the decision to make here is if a missing configuration file constitutes to a bad-enough situation for which we want to interrupt the execution or not. In my opinion it does because the application would behave differently from how I would expect it to do with all the configuration files loaded.

[sampsyo]

I guess I feel pretty agnostic about that precise distinction? But one argument for leaving it as a warning is to be less disruptive to people who were relying on the old behavior, who will now just get a warning when they run beets instead of being prevented from running it altogether… you could have an import in there that just points to a placeholder location that is only sometimes filled with an actual config file, for example.

[adamjakab]

Ok. I see your point. Let's go with a simple warning then.

[adamjakab]

This, with the following configuration:

include: 
    - inexistent.yml

produces:

$ ./beet
Warning! Missing configuration file! [Errno 2] No such file or directory: '/Users/jackisback/.config/beets/DEVEL/inexistent.yml'
Usage: 
  beet COMMAND [ARGS...]
  beet help COMMAND
...

and beets keeps running happily ;)

However, it does not catch badly formatted configuration files. With:

include: 
    - badformat.yml

and with the 'badformat.yml' file:

this
    will
        not
            pass
        as
    ok
yml

I expected the same warning to show because the confuse.load_yaml() function throws a ConfigReadError. It does not. This is what I get:

$ ./beet
configuration error: musicbrainz.host not found

and beets exits. But maybe this needs a separate PR.

[sampsyo]

Great; thanks for continuing to sort this out!

A few random things:

• I wish we could use logging instead of print, but I think that would necessitate an impossible circular import. 😢
• Barring that, let's at least print to stderr instead of stdout.
• I recommend this somewhat terser error message: "configuration `import` failed: "

I am also a tad confused by the issue with the malformed YAML… I would have expected that to throw an error too. Maybe we should dig into why that's not happening!

[adamjakab]

Sure, no worries.

As for the malformed yaml issue I will open a separate issue (also referencing this PR) and we can dig into it. It seems odd.

[sampsyo]

Looking good! Could you please add a quick changelog entry?

[adamjakab]

Could you please add a quick changelog entry?

Hi @sampsyo. This one: docs/changelog.rst? Can you please give me an advice on what editor to use to edit rst files? I have not used this format before and I don't want to mess it up. tnx

[sampsyo]

Looks like you got it; thanks!!

I don't have specific text editor recommendations for editing ReST files (I just use vim 😃), but you might consider previewing changes to the docs. You can generate the HTML files by typing make html in the docs directory, which likely will require you to install Sphinx.