-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
feat(import): Add support for reading skipped paths from logfile #4387
Conversation
f3d0039
to
74e5498
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super cool; thank you for looking into this!
If it doesn't seem overly complex to implement, I think I'd prefer to use an explicit command-line flag (like beet import --read-log foo.log
) as opposed to implicitly allowing logs as arguments alongside files/directories. The main benefit would be that it will be more "debuggable" from the perspective of the user: if the log file isn't want they expect or is malformed, they can get a useful error message instead of letting the text file be treated as a media file. I hope this makes sense!
cb1d45e
to
6c81679
Compare
999b960
to
47cb273
Compare
47cb273
to
3e37d01
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it doesn't seem overly complex to implement, I think I'd prefer to use an explicit command-line flag (like
beet import --read-log foo.log
) as opposed to implicitly allowing logs as arguments alongside files/directories.
Done. I also added a small test for the helper function.
Sorry for the force-pushing, I abused git as a way to deploy the code on my NAS (so I can test it on my library). Also don't have windows so I had to use the CI for trial and error test driven development ;-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome; thanks for taking this "flag" approach! I think this is basically there—I have one or two very low-level comments for your consideration.
ready to review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking great. Thanks for the fast and clean work.
Description
Fixes #4379.
To Do
docs/
to describe it.)docs/changelog.rst
near the top of the document.)