-
Notifications
You must be signed in to change notification settings - Fork 10
Autofix files on import #47
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
Conversation
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.
Thanks for the contribution @OwenCochell. The feature looks good overall but I have some feedback on the implementation. I haven’t had a chance to look at the test code yet but will do after the implementation is fixed.
Co-authored-by: Thomas Scholtes <geigerzaehler@axiom.fm>
|
Just implemented the changes outlined in the comments, I've left some comments for further clarification. All the tests are passing on my end, and hopefully CI agrees. Let me know if there is anything else needed. Thanks! |
Co-authored-by: Thomas Scholtes <geigerzaehler@axiom.fm>
|
Just wanted to comment that last night I have implemented some changes addressing your comments. Let me know if there is anything else needed. Thanks! |
|
Thanks for implementing the new behavior. It works as expected. I still couldn’t fully wrap my head around the fixing logic and had the feeling it could be simplified. So I took the liberty of experimenting with it and pushed a change to your branch. The refactoring avoids deep nesting and should make the control flow easier to follow. Feel free to have another look at it. Otherwise this PR is ready to go and I’ll merge and release a new version soon. |
|
Hello, You changes seem to be equivalent with mine, there are just some minor style and logic changes. This aligns with my original intentions and it gets my approval (whatever that is worth, haha). I eagerly await seeing this merged and included in a new release! Thanks for taking the time to look it over and clean things up. I personally will be using this feature and it will make life much easier. If there is anything else you need let me know. Thanks, |
|
I’ve released 0.15.2 with your changes |
Hello,
I have implemented a feature that will auto-fix corrupted files on import. If a checker reports a problem for a file when it is being imported, then this plugin will attempt the fix the file. This option can be enabled by setting
auto-fix: truein the config file, and by default this functionality is disabled.We fix the file in place, meaning it will be altered during the fix operation. If the file is unable to be fixed, then we will prompt the user asking if they want to skip or not. If quiet mode is enabled, then we will choose to skip by default, which mimics the behavior prior to this change. I have implemented some tests to confirm this, and I made some minor changes to the fixing behavior in the helper so we can confirm if the fix operation was actually applied (I can discuss this in more detail if necessary). I have also updated the readme to reflect these changes.
Finally, I have made the flac checkers more rigorous, the regex now flags warnings are failures, and the flac tool now treats warnings as errors. I also added a fix command for flac files. I understand these changes could be controversial, and it may be best to alter these settings on a per-user basis instead of making it the global default. Let me know what you think.
Please let me know if there is anything else needed! Thanks for creating such a useful project.
Thanks!