Skip to content

Comments

Added option --pretend to only print the filenames#1162

Merged
sampsyo merged 4 commits intobeetbox:masterfrom
mried:import-pretend
Dec 23, 2014
Merged

Added option --pretend to only print the filenames#1162
sampsyo merged 4 commits intobeetbox:masterfrom
mried:import-pretend

Conversation

@mried
Copy link
Contributor

@mried mried commented Dec 21, 2014

Added option --pretend (or -e) to only print the filenames of files to import without actually importing them.

@sampsyo
Copy link
Member

sampsyo commented Dec 21, 2014

Thank you for splitting this out! I'll add a few very low-level comments in-line. Thanks for your patience with this. 🎉

Copy link
Member

Choose a reason for hiding this comment

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

Rather than use this intermediate session.pretend field, we should probably just access session.config['pretend'] for uniformity. To make this work without an explicit membership check, you can add pretend: false to the import section of config_default.yaml.

@sampsyo
Copy link
Member

sampsyo commented Dec 21, 2014

OK, there are my tiny, nit-picking comments. One last thing, which may not be attainable: it's somewhat worrisome that the pretend flag has to appear in four different conditionals. For example, it would be awesome to find a way to make it orthogonal to the "Nothing imported from…" message so that (a) no change would be necessary there, and (b) we'd still get the warning, even in pretend mode. This cleanliness may not be possible to achieve easily, but it's worth pondering.

…only one place where it is read.

Changed the output to use the log system rather than print_.
@mried
Copy link
Contributor Author

mried commented Dec 21, 2014

I think I've worked out all the points you mentioned.
I also had a look if it is possible to reduce the access of the pretend config option and finally succeeded: Right now, there is only one place where the option is read. This has one drawback: Tasks are created and stopped afterwards. Not sure if this is bad since it might be expensive to create them first. Maybe they do some stuff while being created?

@mried
Copy link
Contributor Author

mried commented Dec 22, 2014

Hmm... I tried to change the regexfilefilter plugin to use the import_task_start event. That's working as long as the --pretend flag is not set. Which is obvious since the event is sent when the task starts working. The current implementation of the --pretend flag print out the file names and removes the tasks from the pipeline before the file filter can get to work.
So I see two main solutions:

  • Running the filter before the tasks are created (my old approach with the new plugin hook). This could also lead to a new event like import_task_created.
  • Checking the --pretend flag much later (after the import_task_start was sent) within the task itself.

Any ideas how to solve this?

Copy link
Member

Choose a reason for hiding this comment

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

The e is obsolete here. 😃

@sampsyo
Copy link
Member

sampsyo commented Dec 22, 2014

Fantastic! Thank you for being persistent. This is getting into good shape.

At the risk of being demanding, can I ask for one last iteration? On a closer inspection, I think the best way to get the full orthogonality we need here is to use a separate pipeline stage for --pretend imports. One important advantage to this (in addition to avoiding any need to modify read_tasks itself) will be that this will work for beet import -L QUERY too—that is, it can show the paths for tasks generated by query_tasks.

To see how to do this, look in the run method of ImportSession. There, you can see something like this going on already: if config['autotag'] is false, we switch to a different pipeline stage: import_asis. At this point, we can also check config['pretend'] and tack on a new stage, log_paths or something, and terminate the pipeline there.

Does that make sense? I'd be happy to help with this if you need assistance puzzling through. Let me know and I'll make you a collaborator so you can push to a branch.


Also: Yes, good point about interacting with your new filter idea. A new event like import_task_created may indeed be necessary here after all. Let's chat about this again after finishing --pretend.

Corrected the documentation (shortcut -e is not available any more)
@mried
Copy link
Contributor Author

mried commented Dec 23, 2014

Here we go 😄... This seems to be a very straight forward solution. The only drawback is the creation of the pipeline stages, it's a bit more complex now.

And: Yes, you're right, I forgot to remove the -e within the docs...


BTW: What is the right place to start a chat about the new filter. Should I open an issue?

@sampsyo sampsyo merged commit af36d85 into beetbox:master Dec 23, 2014
sampsyo added a commit that referenced this pull request Dec 23, 2014
Added option --pretend to only print the filenames
sampsyo added a commit that referenced this pull request Dec 23, 2014
@sampsyo
Copy link
Member

sampsyo commented Dec 23, 2014

Awesome! Thank you again for working through this.

Yes, let's talk about the filter in a new issue (or a new PR). Specifically, I think it now should be quite straightforward to implement with (a) a new plugin event, and (b) extending ihate rather than with a separate plugin.

@mried mried deleted the import-pretend branch December 23, 2014 18:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants