plugins: add importhistory #4748
Conversation
|
I have planned to finally look into this PR but I just can't get to it. In the meantime I'd like to mention that I have a quite similar plugin I'm maintaining (original maintainer doesn't use beets anymore). It certainly has a slightly different approach but just wanted to mention and maybe on the long run this could be merged into one plugin (....or not! It might be too different). Also it is pretty old code which I just quickfixed somehow to make it work for my needs. Just want you to have a look if you're interested: https://github.com/JOJ0/beets-dirfields |
doronbehar
force-pushed
the
importhistory-plug
branch
4 times, most recently
from
afafefb
to
26e5ae7
Compare
|
Hey @JOJ0 , after a mostly successful review process, I'd like to bump this PR upwards :) I hopefully fixed the formatting issues, and maybe CI will be green now. I also took a look upon the |
|
I finally found the time to read through the docs. Very useful and well written. Loads of useful usecases! Great! Keeping track of the source path alone is the killerfeature for me❤️ I have one highlevel question regarding that: Will the sourcepath stay untouched in case for some reason I do a reimport from the items already in the library: |
|
Good question, @JOJ0! |
Good question, I had no idea this option existed. Is there a way to know what events this action triggers? |
|
Thanks for all the comments. I'm glad you liked the idea in general, and that we only have to fine-tune the details of the implementation. I think that having tests would be nice, and it'll take me time to delve into it. I'll keep you notified! In the meantime, I pushed a commit fixing the easy issues presented by @sampsyo . |
doronbehar
force-pushed
the
importhistory-plug
branch
2 times, most recently
from
79271b2
to
e2f3ff8
Compare
Best you just test a reimport using that option and see what value source_path has after that. I have a feeling that it might the beets library path and the original source_path gets lost. Give it a try and we'll see if it does. HTH :-) |
|
Hey all, I was busy for a while, and I had some time left today to fix some of the issues discussed back than, including a small bug I encountered while experimenting with the plugin personally. The commits I just pushed fix these issues, and I also marked the review conversations above as resolved, but of course feedback is still welcomed. I didn't have time though, to think thoroughly about @JOJ0's question:
So don't feel like you need to review this now once more - this is still not ready enough. |
doronbehar
force-pushed
the
importhistory-plug
branch
from
c39d244
to
6ba5120
Compare
|
Hmm now I checked and saw that (before the last push) using |
doronbehar
force-pushed
the
importhistory-plug
branch
from
6ba5120
to
1135b73
Compare
|
Now I also fixed the small merge conflict, so this is ready for another round of review :). I think it'd be nice to write tests, but perhaps it'd be good to get your 👍 at this stage. |
|
I would suggest adding a config option to control the prompt to remove the source files, as I suspect there are those that would want to keep information about the import source but want to keep the source files pristine and unmodified otherwise. For example, I keep a copy of all the digital media I own for posterity, even if I remove it from my library, on general principle. Of course, this could always be added later on in a future PR. Other than that, this looks like a great plugin, nice work. |
|
@kergoth I had a similar suggestion in mind...I think.... Do you mean a "global" config option for the plugin that disables all prompts ever and leaves the "source path saving" as the only feature left? Did I get this right? If not please clarify! Thanks! 😀 |
Exactly what I was thinking, yeah. |
doronbehar
force-pushed
the
importhistory-plug
branch
from
bfeb828
to
c475e96
Compare
|
Thanks for the comment :) I rebased to fix the merge conflicts and applied your suggestion. I also revised the documentation. Still I haven't found time yet to add a test for the plugin, and perhaps it'd be nice to do so. |
|
Hi @doronbehar thanks for the quick implementation of that feature! I'm currently reviewing and playing around with it so we can finally get to a merge. Yes, if you feel motivated a test for the plugin would be indeed wonderful! I will push my suggestions directly as commits to your feature branch. Hope that is ok for you, we can discuss here then if you don't like something. It is just much easier for me at the moment to suggest as code/commits directly instead of adding suggestions via GitHub. Again: Hope that's ok, you'll see my commits soon...... PS: Last but not least: I want to apologize for letting you hang there for 6 months already! I was not really "in town" a couple of months ;-) Many thanks for the patience! Let's finally get this done now! 💪 |
and keep one log message in one line.
Check the suggest_removal option value first thing. It does not make sense to move any further in this function
and also state item id.
instead of if/else.
instead of using the decode utf 8 built-in method. Wrap a couple more things with displayable_path that might have been overseen.
JOJ0
force-pushed
the
importhistory-plug
branch
from
d05224b
to
2ac8441
Compare
|
Hi @doronbehar I finished my changes for now. I tried to keep them bite-sized, so you can review them commit by commit. Hope that helps and speak soon. All the best! |
Description
Add a plugin useful for bulk importers.
To Do
docs/to describe it.)docs/changelog.rstnear the top of the document.)