fix #216: angle parameters are not being read from topology file

[jandom]

This will be a fair amount of work actually, to update the parser and support all the different angletypes defined by gromacs... I'm treating this documentation as authoritative source of what to expect for each angletype https://manual.gromacs.org/current/reference-manual/topologies/topology-file-formats.html#tab-topfile2

[jandom]

@orbeckst oh I see that we're still supporting python2.7 in this repo – is that intentional, or do you not actually care?

E     File "/home/runner/work/GromacsWrapper/GromacsWrapper/tests/fileformats/top/test_TOP.py", line 26
E       def create_topology_data() -> str:
E                                  ^
E   SyntaxError: invalid syntax

[orbeckst]

Looking at it, we still do support 2.7 — mainly because it was no real effort to do so. However, that could go by the wayside eventually, but that requires a last release with 2.7, a dedicated cleanup PR etc so for right now, just throw out anything modern and make the tests pass.

(Have a look at #offtopic in the MDAnalysis discord if you want to hear opinions on Python's development cycles...)

[orbeckst]

Please ping me when you need a quick review @jandom . I don't have a lot of free side project time at the moment.

[orbeckst]

And PS: Many thanks for taking on the issue!!!!

[jandom]

No problem! Anybody else that could review the PR in your absence Oli? My one question so far is: do we need to continue and support python 2.7?

…

On Tue, Nov 7, 2023 at 16:19 Oliver Beckstein ***@***.***> wrote: And PS: Many thanks for taking on the issue!!!! — Reply to this email directly, view it on GitHub <#264 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADAVG4RE6F2LBW4G3BXDRDYDJNRJAVCNFSM6AAAAAA66JDKQKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTOOJZGA3TANZYGQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[codecov]

Codecov Report

Merging #264 (6664d77) into main (fa64735) will increase coverage by 0.29%.
The diff coverage is 89.47%.

@@            Coverage Diff             @@
##             main     #264      +/-   ##
==========================================
+ Coverage   68.88%   69.18%   +0.29%     
==========================================
  Files          22       22              
  Lines        3944     3946       +2     
  Branches      708      715       +7     
==========================================
+ Hits         2717     2730      +13     
+ Misses       1047     1036      -11     
  Partials      180      180              

Files	Coverage Δ
gromacs/fileformats/blocks.py	72.35% <100.00%> (+1.38%)	⬆️
gromacs/fileformats/top.py	86.89% <86.04%> (+1.27%)	⬆️

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

[orbeckst]

do we need to continue and support python 2.7?

I am looking at #259 where we/I apparently decided to finally drop 2.7. And I even made the 0.8.5 release that was officially the last one to also support "old Python".

Therefore — you'll be overjoyed to hear — we can rip out any (very) legacy Python ≤ 3.7. I would, however, prefer to do the ripping in a separate PR; for a start just removing the testing in CI and updating the CHANGELOG would be sufficient.

[orbeckst]

If you review PR #265 then you can just use Python ≥3.8 in this PR ;-).

[orbeckst]

Add an entry to CHANGES. Possibly improve testing.

I trust that you actually fixed the issue :-).

[orbeckst]

Can you use pytest's mocking support https://docs.pytest.org/en/7.1.x/how-to/monkeypatch.html so that it stays within pytest? Alternatively, we could install pytest-mock.

[jandom]

ah, what's the motivation here – is it just consistency? I really, really don't enjoy how monkeypatch solves the problem. unittest is mock is basically a built-in and I see it used more frequently.

Happy to add more tests – that's always a good idea – but i will need some direction as to what to cover? My tests already covered all the possible fu values.

[orbeckst]

consistency to stay within one testing framework

[orbeckst]

test coverage itself looks decent enough

[jandom]

If you review PR #265 then you can just use Python ≥3.8 in this PR ;-).

Approved! But of course I coded first and read second so I already factored out the enum import...

[orbeckst]

Please fix the CHANGES but looks good in principle! Thanks a lot @jandom !!

[orbeckst]

Thank you @jandom !!!

[jandom]

Many thanks for your help with this PR @orbeckst – I will note that ChatGPT was heavily used to write this code, with a few adjustments! 🤖

[orbeckst]

I am just relying on you claiming proper authorship of the code and having the knowledge to know when something coming out of a LLM is correct.