Hello @marinegor! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

• In the file package/MDAnalysis/coordinates/MMCIF.py:

Line 28:80: E501 line too long (84 > 79 characters)
Line 41:80: E501 line too long (85 > 79 characters)
Line 42:80: E501 line too long (93 > 79 characters)
Line 61:80: E501 line too long (104 > 79 characters)
Line 65:80: E501 line too long (87 > 79 characters)
Line 67:80: E501 line too long (107 > 79 characters)

• In the file package/MDAnalysis/topology/MMCIFParser.py:

Line 2:24: W291 trailing whitespace
Line 60:80: E501 line too long (111 > 79 characters)
Line 72:80: E501 line too long (123 > 79 characters)
Line 82:80: E501 line too long (122 > 79 characters)
Line 106:80: E501 line too long (108 > 79 characters)
Line 113:80: E501 line too long (80 > 79 characters)
Line 128:80: E501 line too long (91 > 79 characters)
Line 175:80: E501 line too long (126 > 79 characters)
Line 185:80: E501 line too long (125 > 79 characters)
Line 224:80: E501 line too long (126 > 79 characters)
Line 242:80: E501 line too long (140 > 79 characters)
Line 281:80: E501 line too long (87 > 79 characters)
Line 292:80: E501 line too long (90 > 79 characters)

• In the file package/MDAnalysis/topology/PDBParser.py:

Line 56:80: E501 line too long (80 > 79 characters)
Line 57:80: E501 line too long (84 > 79 characters)

• In the file package/MDAnalysis/topology/__init__.py:

Line 335:26: W292 no newline at end of file

• In the file testsuite/MDAnalysisTests/datafiles.py:

Line 48:80: E501 line too long (103 > 79 characters)
Line 81:80: E501 line too long (80 > 79 characters)
Line 97:80: E501 line too long (86 > 79 characters)
Line 271:80: E501 line too long (90 > 79 characters)
Line 340:80: E501 line too long (104 > 79 characters)
Line 387:80: E501 line too long (83 > 79 characters)
Line 436:80: E501 line too long (80 > 79 characters)
Line 463:80: E501 line too long (80 > 79 characters)
Line 481:80: E501 line too long (80 > 79 characters)
Line 493:80: E501 line too long (80 > 79 characters)
Line 494:80: E501 line too long (80 > 79 characters)
Line 497:80: E501 line too long (83 > 79 characters)
Line 498:80: E501 line too long (86 > 79 characters)
Line 546:80: E501 line too long (82 > 79 characters)
Line 547:80: E501 line too long (82 > 79 characters)
Line 549:80: E501 line too long (88 > 79 characters)
Line 551:80: E501 line too long (88 > 79 characters)
Line 552:80: E501 line too long (81 > 79 characters)
Line 777:80: E501 line too long (81 > 79 characters)
Line 778:80: E501 line too long (87 > 79 characters)
Line 779:80: E501 line too long (84 > 79 characters)
Line 780:80: E501 line too long (85 > 79 characters)
Line 781:80: E501 line too long (83 > 79 characters)

Comment last updated at 2024-10-25 11:17:29 UTC

Linter Bot Results:

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

Some issues were found with the formatting of your code.

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/11148966346/job/30986736623

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

@IAlibay If we are are headed for a new release, would love to get this PR merged & part of if if you could take a final look.

+1 on seeing this merged into the next release!

@IAlibay your comment had been addressed and the gemmi reader is also tested in Windows on azure. Could you updated your review, please? Otherwise I'll dismiss it in a day in an effort to merge this important new feature.

(I am not sure what the linter wants, I'll try to make it happy.)

@IAlibay your comment had been addressed and the gemmi reader is also tested in Windows on azure. Could you updated your review, please? Otherwise I'll dismiss it in a day in an effort to merge this important new feature.

(I am not sure what the linter wants, I'll try to make it happy.)

@orbeckst can you give me until Tuesday please? I agree it's an important feature and I do want to review it, however there's some other high priority items within MDAnalysis that needs addressing first.

Either way, I'm not going to be releasing a be version of MDAnalysis until after the 17th - so it not being merged until then isn't an issue.

Ok

Documentation build overview

📚 MDAnalysis | 🛠️ Build #34136622 | 📁 Comparing 177a179 against latest (9358d7a)

  🔍 Preview build  

+ Added

• documentation_pages/coordinates/MMCIF.html
• documentation_pages/topology/MMCIFParser.html
• _modules/MDAnalysis/coordinates/MMCIF.html
• _modules/MDAnalysis/topology/MMCIFParser.html

± Modified

• genindex.html
• index.html
• py-modindex.html
• _modules/index.html
• documentation_pages/coordinates_modules.html
• documentation_pages/overview.html
• documentation_pages/topology_modules.html
• documentation_pages/units.html
• documentation_pages/analysis/align.html
• documentation_pages/analysis/atomicdistances.html
• and 94 more...

Atoms without altLocs in the MMCIF will have null bytes assigned to their altLoc attribute. When writing the structure out to PDB, it includes those null bytes, in violation of the PDB spec (not that this have stopped anyone before). Not fully sure of the consequences but figured I'd raise it before it's merged.

Altered tail of 1BD2.cif to include non-null altLocs:

HETATM 6376 O O   . HOH I 6 .   ? -10.858 27.299 2.616   1.00 63.18  ? 257 HOH E O   1 
HETATM 6377 O O   . HOH I 6 .   ? -11.731 43.369 -12.769 1.00 34.61  ? 258 HOH E O   1 
HETATM 6378 O O   A HOH I 6 .   ? 3.361   39.263 -10.471 1.00 56.34  ? 259 HOH E O   1
HETATM 6379 O O   B HOH I 6 .   ? 3.361   39.263 -10.471 1.00 56.34  ? 259 HOH E O   1
#

Tail of reconstructed PDB (but \0 is actually a null byte)

HETATM 6376  O  \0HOH E 257     -10.858  27.299   2.616  1.00 63.18      E    O  
HETATM 6377  O  \0HOH E 258     -11.731  43.369 -12.769  1.00 34.61      E    O  
HETATM 6378  O  AHOH E 259       3.361  39.263 -10.471  1.00 56.34      E    O  
HETATM 6379  O  BHOH E 259       3.361  39.263 -10.471  1.00 56.34      E    O  
ENDMDL
END

You can recreate this with the following:

https://gist.github.com/ianmkenney/624300e0395bcf2c918c2fa8b92fd86e

Just clone MDA and check out the PR head next to the Makefile and run make test.

Everything should now be addressed - but I can't figure out why codecov is reporting such low test coverage because it's got 100% coverage when I run it locally.