PICARD-2356: Removed deprecated code

[skelly37]

Summary

• This is a…
  • Bug fix
  • Feature addition
  • Refactoring
  • Minor / simple change (like a typo)
  • Other
• Describe this change in 1-2 sentences:
  I've removed code marked in comment as deprecated. 1 commit — 1 function, so everything is clear IMO

Problem

Deprecated code is unnecessarily tested and makes the source code files bloated

• JIRA ticket (optional): PICARD-2356

Solution

Find all the usages of deprecated code, replace it with the new one and safely delete it.

Action

picard.metadata.delete() is also marked as deprecated but are you sure that executing del self[self.normalize_tag(name)] can be enforced safely without such convenient wrapper method? I'm not sure if this is a change for good so I left without replacing delete with del

[phw]

Thanks. Looks good to me.

This will be for Picard 3. We should open a branch for this. I'd like to do this after we have released Picard 2.8

[phw]

@skelly37 CI complains about a now unused import of picard.version.VersionError, otherwise I think everything is good.

[skelly37]

@phw Thanks for feedback, the import is already removed. setup.py test went just fine before, though — would it be possible to paste more such tests into local tests? It'd make the PR process way smoother. If not, I could create a ticket and try to include unnecessary imports check in the tests script

[rdswift]

@skelly37

Thanks for feedback, the import is already removed. setup.py test went just fine before, though — would it be possible to paste more such tests into local tests? It'd make the PR process way smoother

The reason that setup.py test passed is because it doesn't actually lint the python files. You should also use setup.py flake8 and setup.py isort to check your code before submitting. Note that setup.py isort will show two files with errors (picard\resources.py and picard\const\__init__.py) that can safely be ignored. When submitting a pull request, the CI processing for the repository will also check the files with PyLint. This option is not included in setup.py. I've developed a script that I use here to check my files with PyLint before submitting them on a pull request. If you're interested, it can be found on my gist, Picard Developer Script to Check Files Using PyLint.

[skelly37]

@rdswift

@skelly37

Thanks for feedback, the import is already removed. setup.py test went just fine before, though — would it be possible to paste more such tests into local tests? It'd make the PR process way smoother

The reason that setup.py test passed is because it doesn't actually lint the python files. You should also use setup.py flake8 and setup.py isort to check your code before submitting. Note that setup.py isort will show two files with errors (picard\resources.py and picard\const\__init__.py) that can safely be ignored. When submitting a pull request, the CI processing for the repository will also check the files with PyLint. This option is not included in setup.py. I've developed a script that I use here to check my files with PyLint before submitting them on a pull request. If you're interested, it can be found on my gist, Picard Developer Script to Check Files Using PyLint.

Thanks for the advice and decent code. I think that we could benefit from packing setup.py test; setup.py flake8; setup.py isort; $your_script into one. Something like: final_test.py, that also takes care of mentioned errors to ignore.

What do you think?

[rdswift]

What do you think?

Well, the isort, flake8 and test stuff is already there in setup.py, and my script relies on a local file for persisting check information so I'm not sure that it should be included in the Picard repo because it isn't really part of the project. It's just something that I hacked together to help me check my code before submitting (and I still forget to use it occasionally).

[phw]

would it be possible to paste more such tests into local tests? It'd make the PR process way smoother. If not, I could create a ticket and try to include unnecessary imports check in the tests script

I would like to keep running the tests and linting separate. But I guess I would be ok if we would have something like python setup.py lint that would run our current flake8 and isort checks.

Apart from that I would recommend setting up a git pre-commit hook in your local git checkout. There is an example pre-commit hook that runs flake8 and isort on the changed files, see

https://github.com/metabrainz/picard/blob/master/CONTRIBUTING.md#coding-style

Currently there is still at least one unused import left, see

https://github.com/metabrainz/picard/runs/5549909993?check_suite_focus=true

[phw]

Please fix the remaining unused imports.Apart from that I think it would also be good if you could squash all commits into one for this small change, but we can also do this when merging.

[skelly37]

I went through all the modified files and searched with pycharm in all the files. Only import builtins was actually unused.

Moving dateutil.parse import in util/init.py was forced by the pre-commit hook, in case you're wondering why I moved it.

[skelly37]

I've fixed my isort version, no problems this time. The previous commit was deleted, if this time it won't work: I'm clueless.

Okay, the tests finally went smoothly. This string_ in setup.cfg was the problematic one, I see.

[skelly37]

Okay, all the most important changes work. One final question then.

In picard/util/thread.py there's a function called run_task() (line 65). The priority parameter is marked as deprecated but kept for backward compatibility.

My question is: Should I left it intact? Or the backward compatibility there won't be needed?

Solving that case would be the final of this PR imo.

[phw]

My question is: Should I left it intact? Or the backward compatibility there won't be needed?

Good catch. Yes, that could be removed as well. We just keep it for compatibility with older plugins, but since plugins will need an update anyway we can actually change that for Picard 3. That simplifies the code another bit.

[skelly37]

Okay, deprecated parameter got removed. Also, nobody disagreed with my opinion on picard.metadata.delete(), so I deleted the comment. It's still used in code (and useful imo), not just left with declaration and tests.

As long as there won't be more findings before 3.0, we're free from deprecated code! (time will come for the two modules as well)

[phw]

From my side this is good to merge now, thanks a lot.

@zas Are you ok with keeping the Metadata.delete method and remove the deprecation notice there?

[zas]

@zas Are you ok with keeping the Metadata.delete method and remove the deprecation notice there?

Yes.

[phw]

Merged now. We had the 2.8.2 release yesterday and finally should get started as planned to move forward next larger release.