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

Conflict between behaviour & documentation of incremental_skipped_later flag #3315

Open
rulimanurung opened this issue Jun 22, 2019 · 1 comment

Comments

Projects
None yet
2 participants
@rulimanurung
Copy link

commented Jun 22, 2019

See original discussion here: https://discourse.beets.io/t/importing-skipped-albums-incremental-skipped-later-not-being-observed-properly/815

There is a conflict between the behaviour of the incremental_skip_later flag as stated in the documentation here and in the actual implementation here.

The documentation states:

Either yes or no, controlling whether skipped directories are recorded in the incremental list. When set to yes, skipped directories will be recorded, and skipped later. When set to no, skipped directories won’t be recorded, and beets will try to import them again later. Defaults to no.

However, the boolean condition in importer.py is currently:

            # Should we skip recording to incremental list?
            self.skip and session.config['incremental_skip_later']

Thus, if incremental_skip_later is true/yes then it will NOT record skipped directories in the incremental list (and thus NOT skipped upon subsequent imports), but if it is false/no then it WILL record skipped directories in the incremental list (and thus WILL be skipped upon subsequent imports). It looks like this error was introduced in this commit by @jroitgrund.

Problem

$ beet import -qil log.txt /my/audio/folder/
$ beet import -il log.txt /my/audio/folder/

The expectation is that on the second import, since the quiet flag is turned off, beets will try to import the albums that were skipped during the first import. However, it just states No files imported from /my/audio/folder, and skips over everything.

Verified that if I add the following to config.yaml, then it works as expected.

import:
  incremental_skip_later: yes

I suppose this can be fixed by either changing the documentation or changing the boolean condition in importer.py. To be honest, I'm not entirely sure what should be the expected behaviour given the flag name "incremental_skip_later". It's quite confusing.

@sampsyo sampsyo added the bug label Jun 22, 2019

@sampsyo

This comment has been minimized.

Copy link
Member

commented Jun 22, 2019

Arg! What a mess. This is indeed very confusing and we’ve obviously messed up the docs.

Here's a brief history of where all this came from:

  • Before all this started, we always recorded everything, regardless of whether or not the user skipped it.
  • The original implementation of the config option comes from #2773. That seems to have been incorrect, and when the option was turned on, it never recorded anything.
  • The current behavior was introduced in 5f26ec6. This means that when the option is off (the default), the behavior is the same as it was before the option even existed (i.e., we record everything all the time). And when it's on, you get a chance to come back to skipped music later. This seems to match the name.
  • The docs were somewhat unclear. So #3187 attempted to make it clearer, but in doing so, it got the behavior exactly backward.

So I think we should fix the docs, not the code. It needs to say the opposite of what it currently says. And hopefully we can make the actual use case more obvious in the process.

@sampsyo sampsyo added docs and removed bug labels Jun 22, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.