Add auto_keep option to convert plugin - attempt to fix #1840 #4302

DucNg · 2022-02-28T13:26:36Z

Description

My idea was to add a new option so people using the old option won't have to change anything. This way it's a non breaking change. Even if being the default for the auto option would make more sens.

I don't think option naming is ideal so I'm very open to suggestions on this.

Using beet-alternatives to do exactly that is also a possibility but seemed a little overkill for my use case. Figured could be nice to have it build in.

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.)

Should I had tests for this? Changes seems minimal.

sampsyo · 2022-03-01T17:54:51Z

Thanks for getting this started; this seems like a good approach! (That is, an entirely separate option that's like auto but does the other thing with the generated files.)

I actually don't mind the name. auto_keep seems perfectly serviceable to me.

Here is an ill-posed question, if you don't mind: I'm curious about why there is so much code in the new auto_convert_keep method. It needs to cover all the configuration bases, of course. But I wonder if this would be a good opportunity to refactor some of that stuff out so that it can share setup/configuration logic with the other two modes (auto and explicitly with the beet convert command). But how to do this is not immediately obvious and would require some creativity…

DucNg · 2022-03-01T18:10:53Z

You're right, all the new code is basically copy paste from the convert function. Now might be a good time to refactor. I'll see what I can do.

…

-------- Original Message --------

On Mar 1, 2022, 18:55, Adrian Sampson wrote: Thanks for getting this started; this seems like a good approach! (That is, an entirely separate option that's like auto but does the other thing with the generated files.) I actually don't mind the name. auto_keep seems perfectly serviceable to me. Here is an ill-posed question, if you don't mind: I'm curious about why there is so much code in the new auto_convert_keep method. It needs to cover all the configuration bases, of course. But I wonder if this would be a good opportunity to refactor some of that stuff out so that it can share setup/configuration logic with the other two modes (auto and explicitly with the beet convert command). But how to do this is not immediately obvious and would require some creativity… — Reply to this email directly, [view it on GitHub](#4302 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/AED7DWR3PI5IDA3OVPUIKK3U5ZKXRANCNFSM5PRC2FPA). Triage notifications on the go with GitHub Mobile for [iOS](https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675) or [Android](https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub). You are receiving this because you authored the thread.Message ID: ***@***.***>

convert items after imports but also keep original files intact

DucNg · 2022-03-04T09:45:18Z

beetsplug/convert.py

-            link = self.config['link'].get(bool)
-
-            threads = self.config['threads'].get(int)
+            empty_opts = self.commands()[0].parser.get_default_values()


Is there a better way to do this? It doesn't feel quite right.

Very creative! While it doesn't bother me too much, it does seem a little odd that we'd be using the CLI arguments here even though this method doesn't have anything to do with the command itself.

It's not exactly very clean, but one option would be to let _get_opts_and_config's argument be None. I think all of the conditionals in there would end up ignoring the opts value for this mode anyway, right? At least it would make it clear that we are using only the configuration values, nothing from the command line.

Actually that was my first attempt but it doesn't work.

For example for dest it will do:
dest = None.dest ....
Resulting in: 'NoneType' object has no attribute 'dest'

A elegant workaround would have been to to this: dest = opts?.dest for every options. But unfortunately safe navigation operator is not available in python.

The only option left then would be to manually check if opts is None for every option witch is not very elegant in my opinion. What do you think? I think parser.get_default_values() works well in this context but looks very hacky at the same time.

Ah, yes, the opts.foo if opts else config[‘foo’].get() dance was pretty much what I was recommending here. But you make a good point that this would bring a high complexity tax for something that doesn’t seem worth it.

I think a better solution would probably be to use Confuse’s support for argparse options. But maybe the best route would be to note that this is the next step and merge this PR as-is, hoping to tackle the Confuse-based alternative in a subsequent PR (since it will involve some refactoring of its own)?

Fine with me

DucNg · 2022-03-04T09:45:48Z

@sampsyo I tried to refactor common code as best as I could.

sampsyo

Awesome; looking good!! Here are a few granular suggestions.

sampsyo · 2022-03-04T20:14:19Z

docs/plugins/convert.rst

+- **auto_keep**: As opposite to **auto**, import non transcoded versions of
+  your files but still convert them to **dest**. It uses the default format
+  you have defined in your config file.
+  Default: ``no``.


Maybe this should be slightly more explicit that this does this automatically on import? In that sense, it is similar to (rather than solely different from) auto.

sampsyo · 2022-03-04T20:14:28Z

beetsplug/convert.py

+            hardlink = self.config['hardlink'].get(bool)
+            link = self.config['link'].get(bool)
+
+        return (dest, threads, path_formats, fmt, pretend, hardlink, link)


No parentheses are necessary here.

sampsyo · 2022-03-04T20:15:01Z

beetsplug/convert.py

@@ -546,3 +526,45 @@ def _cleanup(self, task, session):
                if os.path.isfile(path):
                    util.remove(path)
                _temp_files.remove(path)
+
+    def _get_opts_and_config(self, opts):


A quick docstring on this new method would be super helpful. (No need to even list all the options exhaustively, IMO—just giving the gist of what this does would be useful.)

sampsyo · 2022-03-04T20:15:11Z

beetsplug/convert.py

+    def _parallel_convert(self, dest, keep_new, path_formats, fmt,
+                          pretend, link, hardlink, threads, items):


A docstring here could be really useful too.

sampsyo · 2022-03-04T20:20:36Z

beetsplug/convert.py

-            link = self.config['link'].get(bool)
-
-            threads = self.config['threads'].get(int)
+            empty_opts = self.commands()[0].parser.get_default_values()


Very creative! While it doesn't bother me too much, it does seem a little odd that we'd be using the CLI arguments here even though this method doesn't have anything to do with the command itself.

It's not exactly very clean, but one option would be to let _get_opts_and_config's argument be None. I think all of the conditionals in there would end up ignoring the opts value for this mode anyway, right? At least it would make it clear that we are using only the configuration values, nothing from the command line.

sampsyo

OK, this is good to go! Seeking volunteers for a broader refactoring of the way configuration works for this plugin…

mannp · 2022-03-12T14:13:19Z

Thanks for the additional option... I wondered if the new option respects if auto convert is disabled?

I have the following config, which I would not expect to convert new imported items, but it is?

convert:
    auto: no
    auto_keep: True,

sampsyo · 2022-03-12T16:47:36Z

It's a good point; it might be nice to document the interaction between the two options. The intent is that they're mutually exclusive; you enable one or the other. Care to expand on that in the docs at all, @DucNg?

mannp · 2022-03-12T17:18:45Z

I'd understood 'auto' was to convert or not to convert on import... while I convert by running the convert command... if it's working the same as auto, it would still keep the originals in place, but only do it when the convert command is issued.

Will auto_keep: 'no' also work, to keep it the same as auto?

Just observations, but thanks again for making the changes :)

sampsyo · 2022-03-12T22:17:53Z

Yes! The default for auto_keep is no, so you don't to change anything if you don't want to use it. But keeping it off will let you manually convert stuff by typing beet convert.

DucNg · 2022-03-14T23:18:18Z

It's a good point; it might be nice to document the interaction between the two options. The intent is that they're mutually exclusive; you enable one or the other. Care to expand on that in the docs at all, @DucNg?

Yes that might be a good idea. Even if enabling both is theoretically possible.

DucNg added 3 commits March 4, 2022 10:40

add option to convert items after import

18e8b73

convert items after imports but also keep original files intact

doc + linter + changelog

af5858d

refactoring

a2325e7

DucNg force-pushed the auto_keep branch from 424a048 to a2325e7 Compare March 4, 2022 09:42

DucNg commented Mar 4, 2022

View reviewed changes

sampsyo reviewed Mar 4, 2022

View reviewed changes

add some documentation

c4281dd

DucNg requested a review from sampsyo March 9, 2022 17:35

Expand changelog for beetbox#4302

03499a3

sampsyo approved these changes Mar 11, 2022

View reviewed changes

sampsyo merged commit bf66895 into beetbox:master Mar 11, 2022

DucNg deleted the auto_keep branch March 11, 2022 18:02

DucNg mentioned this pull request Mar 16, 2022

Expand documentation on auto and auto_keep #4322

Closed

snejus mentioned this pull request Jun 4, 2024

Fix the version error in the release workflow #5274

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add auto_keep option to convert plugin - attempt to fix #1840 #4302

Add auto_keep option to convert plugin - attempt to fix #1840 #4302

DucNg commented Feb 28, 2022

sampsyo commented Mar 1, 2022

DucNg commented Mar 1, 2022 via email

DucNg Mar 4, 2022

sampsyo Mar 4, 2022

DucNg Mar 9, 2022

sampsyo Mar 9, 2022

DucNg Mar 10, 2022

DucNg commented Mar 4, 2022

sampsyo left a comment

sampsyo Mar 4, 2022

sampsyo Mar 4, 2022

sampsyo Mar 4, 2022

sampsyo Mar 4, 2022

sampsyo Mar 4, 2022

sampsyo left a comment

mannp commented Mar 12, 2022

sampsyo commented Mar 12, 2022

mannp commented Mar 12, 2022

sampsyo commented Mar 12, 2022

DucNg commented Mar 14, 2022

		def _parallel_convert(self, dest, keep_new, path_formats, fmt,
		pretend, link, hardlink, threads, items):

Add auto_keep option to convert plugin - attempt to fix #1840 #4302

Add auto_keep option to convert plugin - attempt to fix #1840 #4302

Conversation

DucNg commented Feb 28, 2022

Description

To Do

sampsyo commented Mar 1, 2022

DucNg commented Mar 1, 2022 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DucNg commented Mar 4, 2022

sampsyo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sampsyo left a comment

Choose a reason for hiding this comment

mannp commented Mar 12, 2022

sampsyo commented Mar 12, 2022

mannp commented Mar 12, 2022

sampsyo commented Mar 12, 2022

DucNg commented Mar 14, 2022