Database parental directories creation if not exist by alicezou · Pull Request #4327 · beetbox/beets

alicezou · 2022-03-27T14:51:34Z

Description

Add prompt for users to select whether to create parental directories when they do not exist.

To Do

Documentation. (If you've add a new command-line flag, for example, find the appropriate page under docs/ to describe it.)
Changelog. (Add an entry to docs/changelog.rst near the top of the document.)
Tests. (Encouraged but not strictly required.) [still working on it]

wisp3rwind

Hi, thanks for getting this started! I've left a few inline comments on how I think the design of this PR might be improved.

wisp3rwind · 2022-03-29T20:05:25Z

+    # 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)
+


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?

wisp3rwind · 2022-03-29T20:08:28Z

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


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, ...?

wisp3rwind · 2022-03-29T20:10:06Z

+        try:
+            dbcore.Database(non_exist_path)
+        except OSError as e:
+            raise e


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

wisp3rwind · 2022-03-29T20:15:01Z

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


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.

wisp3rwind · 2022-03-29T20:15:07Z

+            raise e
+        shutil.rmtree("ParentalDirCreationTest")
+
+    @mock.patch('builtins.input', side_effect=['n', ])


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

alicezou · 2022-03-30T01:43:12Z

Hi Benedikt,
Thank you for your review! I have updated my commits.

sampsyo

Looking great!! Here are a few granular suggestions.

I actually think it would be fine (and a bit simpler) to keep the prompt code directly in the beets.ui module (i.e., ui/__init__.py) rather than having a short function in the beets.ui.commands module. The latter module is centered around providing implementations for the beet <foo> subcommands, and this functionality is actually a global thing that is not associated with any particular subcommand—so it would be a bit simpler to just keep everything in the new _check_db_directory_exists function.

I also just wanted to state for the record that this issue will probably only affect people who have changed the library configuration option… beets automatically creates ~/.config/beets by default and puts library.db in there. But custom database locations would not enjoy this automation (until now).

sampsyo · 2022-03-30T15:35:46Z

+def database_dir_creation(path):
+    # Ask the user for a choice.
+    return ui.input_yn("The database directory {} does not \
+                       exists, create it (Y/n)?"


Suggested change

exists, create it (Y/n)?"

exists. Create it (Y/n)?"

So it's two full sentences.

sampsyo · 2022-03-30T15:36:40Z

+* Create the parental directories for database if they do not exist.



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.

sampsyo · 2022-03-30T15:37:20Z

 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


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.

alicezou · 2022-03-30T16:57:59Z

Hi Adrian,
Thank you for the feedback! I have updated the related files.

alicezou · 2022-04-06T21:39:15Z

Hi,
It's been a week since my last update. Are there any changes that I can make to improve this pull request? I am looking forward to hearing from you. :)
@wisp3rwind @sampsyo

sampsyo

Looking good!! I have just a few very small comments, but then I think this should be good to go.

sampsyo · 2022-04-07T18:54:35Z

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


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

sampsyo · 2022-04-07T18:55:23Z

+def database_dir_creation(path):
+    # Ask the user for a choice.
+    return input_yn("The database directory {} does not \
+                       exists. Create it (Y/n)?"
+                    .format(util.displayable_path(path)))


I suggest just "inlining" this code, since it's pretty simple and not called anywhere else. (That is, just do this prompting right inside the function below instead of having a separate function.)

sampsyo · 2022-04-07T18:55:37Z

+                    .format(util.displayable_path(path)))
+
+
+def _check_db_directory_exists(path):


Maybe should be called _ensure_db_directory_exists since it does more than just check?

sampsyo · 2022-04-07T18:55:45Z

+def database_dir_creation(path):
+    # Ask the user for a choice.
+    return input_yn("The database directory {} does not \
+                       exists. Create it (Y/n)?"


exists -> exist

alicezou · 2022-04-07T19:24:39Z

Hi Adrian,
Thank you for your comments! I have updated the relevant parts.

sampsyo

Looks perfect. Thanks again!!

alicezou added 3 commits March 27, 2022 09:56

create db without prompt

9917f2a

add prompt

b07b0e2

style checker, change log

c67245e

alicezou mentioned this pull request Mar 27, 2022

Error when library database directory doesn't exist #3808

Closed

alicezou added 3 commits March 27, 2022 10:58

change doc

d3d9318

fix test cases, support in memory db

879ed7f

add test cases

c0d05f8

alicezou marked this pull request as ready for review March 27, 2022 21:11

check bytes

9029a8a

alicezou mentioned this pull request Mar 28, 2022

Cannot run tests on an Ubuntu 20.04.3 AWS server #4328

Closed

wisp3rwind requested changes Mar 29, 2022

View reviewed changes

alicezou added 3 commits March 29, 2022 19:58

working db

a0b0028

switch to control_stdin

67e778f

fix code review comments

2886296

sampsyo reviewed Mar 30, 2022

View reviewed changes

change location for database_dir_creation, change docs

b609cae

sampsyo requested changes Apr 7, 2022

View reviewed changes

change func name, inline function, fix typo

fa5862d

sampsyo approved these changes Apr 7, 2022

View reviewed changes

sampsyo merged commit e977a6f into beetbox:master Apr 7, 2022

snejus mentioned this pull request Jun 4, 2024

Fix the version error in the release workflow #5274

Merged

snejus mentioned this pull request Dec 5, 2024

Release: Fix changelog formatting #5529

Merged

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

	* 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`

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

		.format(util.displayable_path(path)))


		def _check_db_directory_exists(path):

Uh oh!

Conversation

alicezou commented Mar 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

To Do

Uh oh!

wisp3rwind left a comment

Choose a reason for hiding this comment

Uh oh!

wisp3rwind Mar 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alicezou commented Mar 30, 2022

Uh oh!

sampsyo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alicezou commented Mar 30, 2022

Uh oh!

alicezou commented Apr 6, 2022

Uh oh!

sampsyo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alicezou commented Apr 7, 2022

Uh oh!

sampsyo left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

alicezou commented Mar 27, 2022 •

edited

Loading

wisp3rwind Mar 29, 2022 •

edited

Loading