Addition of pytest case for not None values for frames and start/stop/step

[talagayev]

Fixes #4648

Changes made in this Pull Request:

• Added a pytest case in test_base.py to cover the case where frames and start/stop/step are both specified as not None, leading to an ValueError

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.

---

📚 Documentation preview 📚: https://mdanalysis--4769.org.readthedocs.build/en/4769/

[pep8speaks]

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

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2024-11-27 16:01:33 UTC

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.66%. Comparing base (abc9806) to head (ba62187).
Report is 2 commits behind head on develop.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #4769      +/-   ##
===========================================
- Coverage    93.68%   93.66%   -0.03%     
===========================================
  Files          177      189      +12     
  Lines        21742    22808    +1066     
  Branches      3055     3055              
===========================================
+ Hits         20368    21362     +994     
- Misses         927     1000      +73     
+ Partials       447      446       -1     

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

[hmacdope]

LGTM @talagayev!

[IAlibay]

Happy to discuss this further, but would prefer if we didn't fill up the changelog with small test changes.

[talagayev]

now that I was working on a different Issue I noticed that test_frame_fail in test_base.py possibly already covers the Issue #4648

[RMeli]

@talagayev can you please resolve the conflict? Thanks.

[talagayev]

@RMeli it's now good to go :)

[RMeli]

Thanks @talagayev.

Note for other reviewers: formatting will be sorted down the line, so I'm approving as-is.

[talagayev]

Thanks @talagayev.

Note for other reviewers: formatting will be sorted down the line, so I'm approving as-is.

Happy to help :)

For the formatting for any upcoming PRs, I can run black through the added code if this would be preferred, or is
there any preference if it is formatted down the line ?

[RMeli]

For the formatting for any upcoming PRs, I can run black through the added code if this would be preferred, or is there any preference if it is formatted down the line ?

Up to you, but in such case black should only affect the added code, not the whole file. We are using black too, as set up in #4725. However, we are formatting one module at a time so that we can actually check what black is doing.

So either way is fine. If you don't format what you are writing, it will be automatically formatted with black eventually.

[talagayev]

For the formatting for any upcoming PRs, I can run black through the added code if this would be preferred, or is there any preference if it is formatted down the line ?

Up to you, but in such case black should only affect the added code, not the whole file. We are using black too, as set up in #4725. However, we are formatting one module at a time so that we can actually check what black is doing.

So either way is fine. If you don't format what you are writing, it will be automatically formatted with black eventually.

Ah I see, will then run it for the code snippets that I add, although I got a feeling that sometimes PEP8 has some small complaints for some black formatting

[RMeli]

Yes, this page details how to use black with other tools: https://black.readthedocs.io/en/stable/guides/using_black_with_other_tools.html

I'll give @IAlibay a chance to review, but don't worry about the formatting.

[talagayev]

Yes, this page details how to use black with other tools: https://black.readthedocs.io/en/stable/guides/using_black_with_other_tools.html

I'll give @IAlibay a chance to review, but don't worry about the formatting.

Ah I see, thanks for the link, will take a look at it :)

[IAlibay]

Sorry didn't realise I was blocking here, lgtm.