Substitute plugin #4668
Substitute plugin #4668
Conversation
docs/changelog.rst
Outdated
| @@ -8,6 +8,7 @@ Changelog goes here! | |||
|
|
|||
| New features: | |||
|
|
|||
| * :doc:`/plugins/substitute`: Add the substitute plugin to solve `2786` | |||
There was a problem hiding this comment.
Please provide a little description of the change introduced and then indicate :bug:2786` as additional information.
There was a problem hiding this comment.
I changed the description in changelog
There was a problem hiding this comment.
Thanks for getting this started!! I have a couple of suggestions in here. The main one is that I think the documentation could use some examples of how to use the plugin (not just how to configure it), and I think it would benefit from merely linking to the rewrite plugin's doc page instead of duplicating text, where there is overlap.
|
Hi, I left some wording suggestions whithin. Other than that I read the documentation an I think now it is clear what it does and how it should be used. All the corrections where worth it I guess. Great plugin idea btw! Cool! I approved the checks to run (required for first-time-contributors) and now you should see how to make the linter happy :-) HTH |
|
It looks like the style checker and the docs compilation may need some attention. |
|
@fdaniele85 this was very close to ready for merge, can I assist with the remaining linting issues? Do you see the errors? Sometimes it's hard to find in the github actions verbose outputs. |
|
It would be great if you can help me!
Il dom 14 mag 2023, 12:55 J0J0 Todos ***@***.***> ha scritto:
… @fdaniele85 <https://github.com/fdaniele85> this was very close to ready
for merge, can I assist with the remaining linting issues? Do you see the
errors? Sometimes it's hard to find in the github actions verbose outputs.
—
Reply to this email directly, view it on GitHub
<#4668 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AB3C2AKLONKOVLLAYCVWEFDXGC22FANCNFSM6AAAAAAUUBSPZI>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
As a first step, let's addres the flake8 linter issues: In the list on the very bottom of this PR that says "Some checks were not succesful", click on the "Details" button of the very last entry: ci / lint (pull_reqest) You will see the linting errors. Here's a screenshot:
You will find almost all information on how to fix those in PEP8's style guide: https://peps.python.org/pep-0008/ For example, this describes the E302 error: https://peps.python.org/pep-0008/#blank-lines The D100 and 101 errors are easy to fix as well: Just put a short docstring on top of substitute.py and another one on top of your plugin's class. If you are interested describes it in very longish ways: https://peps.python.org/pep-0257/#specification What you can also do while fixing them: Run flake8 locally on your plugins' Python file: That should help to check if you were successful fixing before you commit and push and let the beets ci pipeline run again (it takes a couple of minutes to finish, we have a lot of tests and checks ;-) |
|
Now I do not get errors executing |
|
I see other problems, but this is a screenshot of my flake8 Is there a method to perform the same testing in local? |
|
The current issues are actually with the tests and docs (in addition to the linter, which may just be due to a different version). You can click through on a specific run to see the details! 
Currently, the docs builder says: |
|
There's some linter issues left @fdaniele85 Hope that helps :-) |
|
Hi @fdaniele85 would it help if I hijack your branch and fix those issues with the docs build? Or is it just a matter of finding time on your end? We are so close to merge-ready here :-))) |
It would be grate @JOJ0! |
JOJ0
force-pushed
the
substitute_plugin
branch
from
5122559
to
993fcb1
Compare
While rebasing master into this feature branch I removed fdaniele85's original version(s) of the changelog to make conflict resolvement easier. This is a slightly extended version of the latest version I found in the original commits.
JOJ0
force-pushed
the
substitute_plugin
branch
from
993fcb1
to
442a535
Compare
in the plugin docs as was discovered in beetbox#2786 (Metadata being modified).
Hi @fdaniele85, I finally found time to take over your branch and fix those linting and docs issues we see in the failed checks. I rebased master into your branch, leaving out the changelog because of many conflicts, I then re-added the changelog with a slightly extended wording. Hope it's according to what you had in mind? Furthermore since as you discovered in your intial report in #2786 and as @sampsyo and @wisp3rwind elaborate here I also tried to include a fix of the misleading documentation of the Please, the 3 of you, read back this paragraph I drafted and suggest how we could put it to not sound "clumsy" and technically correct: 54efb27 |
to finally make linter happy.
|
@JOJ0 thanks a lot for your updates! They are good to me, I did not well understand why |
on whether it modifies metadata or not. Let's also leave a link to the issue here to make it superclear and researchable for anyone stumbling across it. Also suggest the substitute plugin as an alternative.
|
I did a final rewording - probably not perfect english but I guess "good enough" - I also left a link to your initial issue @fdaniele85, as well as a link to this new plugin. Many thanks for the submission to you and sorry that it took so long; but as so often: In the end it was all worth it :-) |
Description
Fixes #2786.
Add the substitute plugin that is a replacement to rewrite plugin. Indeed, rewrite plugin modifies the metadata tags of the file, this plugin does not modify the metadata.
To Do
Tests.