Move ConverterBase out of coordinates/base.py

[ianmkenney]

Fixes #3404

Changes made in this Pull Request:

• Moved ConverterBase into converters/base.py

PR Checklist

• Tests?
• Docs?
• CHANGELOG updated?
• Issue raised/referenced?

Developers certificate of origin

• I certify that this contribution is covered by the LGPLv2.1+ license as defined in our LICENSE and adheres to the Developer Certificate of Origin.

[IAlibay]

We can't just purely move this out of here without some kind of deprecation window.

Could you maybe subclass the ConverterBase here and add a warning in __init_subclass__?

[ianmkenney]

Unfortunately that leads to a circular reference. Instead I include the code in both base.py files but have a deprecation warning when the ConverterBase from coordinates is subclassed.

[orbeckst]

This solution works for me and the comments make clear what to do. I also really don't have a more elegant solution.

[IAlibay]

Ok that makes sense - we definitely need tests to check that each respective subclass does / doesn't raise the warning.

[github-actions]

Linter Bot Results:

Hi @ianmkenney! Thanks for making this PR. We linted your code and found the following:

Some issues were found with the formatting of your code.

Code Location	Outcome
main package	⚠️ Possible failure
testsuite	⚠️ Possible failure

Please have a look at the darker-main-code and darker-test-code steps here for more details: https://github.com/MDAnalysis/mdanalysis/actions/runs/6016355860/job/16320167782

---

Please note: The black linter is purely informational, you can safely ignore these outcomes if there are no flake8 failures!

[codecov]

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.01% ⚠️

Comparison is base (957430b) 93.40% compared to head (6eb6711) 93.39%.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #4253      +/-   ##
===========================================
- Coverage    93.40%   93.39%   -0.01%     
===========================================
  Files          169      184      +15     
  Lines        22204    23333    +1129     
  Branches      4064     4065       +1     
===========================================
+ Hits         20740    21793    +1053     
- Misses         948     1024      +76     
  Partials       516      516              

Files Changed	Coverage Δ
package/MDAnalysis/converters/ParmEd.py	90.28% <100.00%> (+0.05%)	⬆️
package/MDAnalysis/converters/RDKit.py	98.97% <100.00%> (+<0.01%)	⬆️
package/MDAnalysis/converters/base.py	100.00% <100.00%> (ø)
package/MDAnalysis/coordinates/base.py	94.09% <100.00%> (-0.50%)	⬇️

... and 14 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[orbeckst]

Looks good but needs a test for the deprecation. See other tests that check for the DeprecationWarning and the specific message.

[orbeckst]

This solution works for me and the comments make clear what to do. I also really don't have a more elegant solution.

[IAlibay]

Ok that makes sense - we definitely need tests to check that each respective subclass does / doesn't raise the warning.

[orbeckst]

Thanks for adding the tests. You addressed everything that I had seen. I think you also addressed all of @IAlibay 's comments but I leave it to him to judge.

I checked the docs and they look fine. I am not 100% convinced that we need a versionadded 2.7.0 for the MDAnalysis.converters.base docs because it might be a bit confusing as it really had been present since 2.0.0, just not in this place, but I could be convinced either way and leave this to you if you want to add it.

[IAlibay]

I checked the docs and they look fine. I am not 100% convinced that we need a versionadded 2.7.0 for the MDAnalysis.converters.base docs because it might be a bit confusing as it really had been present since 2.0.0, just not in this place, but I could be convinced either way and leave this to you if you want to add it.

I would assume a versionchanged rather than added (if you were to add the directive at all)?

[IAlibay]

@ianmkenney @orbeckst - yes my comments have been addressed, but I'm going to keep the block for now because I think we're back in another merge freeze (for non bugfixes).

[IAlibay]

lgtm - unblocking now the release is done