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

Database parental directories creation if not exist #4327

Merged
merged 12 commits into from
Apr 7, 2022
11 changes: 11 additions & 0 deletions beets/dbcore/db.py
Original file line number Diff line number Diff line change
Expand Up @@ -901,8 +901,19 @@ class Database:
data is written in a transaction.
"""

# Check whether parental directories exist.
def _path_checker(self, path):
if not isinstance(path, bytes) and path == ':memory:': # in memory db
return
newpath = os.path.dirname(path)
if not os.path.isdir(newpath):
from beets.ui.commands import database_dir_creation
if database_dir_creation(newpath):
os.makedirs(newpath)

Copy link
Member

@wisp3rwind wisp3rwind Mar 29, 2022

Choose a reason for hiding this comment

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

It is somewhat unfortunate that this couples the beets.dbcore and beets.ui modules, whereas the former used to be standalone (except for importing beets.util). Would it be possible instead to catch the exception on missing directory in beets/ui/__init__.py:_open_library(), ask for creating the directory, and trying to open the database again afterwards?

Apart from that, a more descriptive name instead _path_checker what be nice, maybe along the lines of check_db_directory_exists?

def __init__(self, path, timeout=5.0):
self.path = path
self._path_checker(path)
self.timeout = timeout

self._connections = {}
Expand Down
9 changes: 9 additions & 0 deletions beets/ui/commands.py
Original file line number Diff line number Diff line change
Expand Up @@ -1398,6 +1398,15 @@ def show_version(lib, opts, args):
version_cmd.func = show_version
default_commands.append(version_cmd)

# database_location: return true if user
# wants to create the parent directories.

Copy link
Member

Choose a reason for hiding this comment

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

This comment looks like it's out of date (it can just be deleted).


def database_dir_creation(path):
# Ask the user for a choice.
return ui.input_yn("{} does not exist, create it (Y/n)?"
Copy link
Member

Choose a reason for hiding this comment

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

It might be helpful to more explicity describe the issue here; the path alone might not be enough to understand the problem: The database directory {} does not exists, ...?

.format(displayable_path(path)))


# modify: Declaratively change metadata.

Expand Down
1 change: 1 addition & 0 deletions docs/changelog.rst
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ Changelog
Changelog goes here!

New features:
* Create the parental directories for database if they do not exist.

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Create the parental directories for database if they do not exist.
* Create the parental directories for database if they do not exist.
:bug:`3808` :bug:`4327`

Including references to where this was discussed.

* :ref:`musicbrainz-config`: a new :ref:`musicbrainz.enabled` option allows disabling
the MusicBrainz metadata source during the autotagging process
Expand Down
3 changes: 2 additions & 1 deletion docs/guides/main.rst
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,8 @@ place to start::

Change that first path to a directory where you'd like to keep your music. Then,
for ``library``, choose a good place to keep a database file that keeps an index
of your music. (The config's format is `YAML`_. You'll want to configure your
of your music. Beets will prompt you if the parental directories for database do
not exist. (The config's format is `YAML`_. You'll want to configure your
Copy link
Member

Choose a reason for hiding this comment

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

TBH I think it's probably OK to leave this sentence off? This is an edge case, and the prompt clearly explains to users what's going on, so I don't think we need to warn people that this might happen.

text editor to use spaces, not real tabs, for indentation. Also, ``~`` means
your home directory in these paths, even on Windows.)

Expand Down
25 changes: 25 additions & 0 deletions test/test_dbcore.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@
import shutil
import sqlite3
import unittest
from random import random
from unittest import mock

from test import _common
from beets import dbcore
Expand Down Expand Up @@ -760,8 +762,31 @@ def test_no_results(self):
ModelFixture1, dbcore.query.FalseQuery()).get())


class ParentalDirCreation(unittest.TestCase):
@mock.patch('builtins.input', side_effect=['y', ])
def test_create_yes(self, _):
non_exist_path = "ParentalDirCreationTest/nonexist/" + str(random())
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be using a path within a temporary directory, to be certain that we don't pollute the environment where the test is run.

try:
dbcore.Database(non_exist_path)
except OSError as e:
raise e
Copy link
Member

Choose a reason for hiding this comment

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

That would behave identically with the try ... catch removed, right?

shutil.rmtree("ParentalDirCreationTest")

@mock.patch('builtins.input', side_effect=['n', ])
Copy link
Member

Choose a reason for hiding this comment

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

It might be easier to use the control_stdin context manager from test/helper.py (see its docstring for details).

def test_create_no(self, _):
non_exist_path = "ParentalDirCreationTest/nonexist/" + str(random())
try:
dbcore.Database(non_exist_path)
except OSError as e:
raise e
if os.path.exists("ParentalDirCreationTest/nonexist/"):
shutil.rmtree("ParentalDirCreationTest")
raise OSError("Should not create dir")


def suite():
return unittest.TestLoader().loadTestsFromName(__name__)


if __name__ == '__main__':
unittest.main(defaultTest='suite')