Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix inconsistent restriction digest cutting behaviour #4638

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

manulera
Copy link
Contributor

@manulera manulera commented Feb 27, 2024

  • I hereby agree to dual licence this and any previous contributions under both
    the Biopython License Agreement AND the BSD 3-Clause License.

  • I have read the CONTRIBUTING.rst file, have run pre-commit
    locally, and understand that continuous integration checks will be used to
    confirm the Biopython unit tests and style checks pass with these changes.

  • I have added my name to the alphabetical contributors listings in the files
    NEWS.rst and CONTRIB.rst as part of this pull request, am listed
    already, or do not wish to be listed. (This acknowledgement is optional.)

Closes #4604

@peterjc I have also refactored the code, since there was some code repeated up to three times in the different _drop functions. Let me know if that makes sense.

@manulera
Copy link
Contributor Author

manulera commented Feb 27, 2024

Hi @peterjc I am not sure test that does not pass is actually wrong. The failed test can be reproduced with python run_tests.py "Bio.Restriction".

I am not sure, but If I understand the error correctly, it is checking that the output of a.print_that() in the docstring of the file Bio/restricntion/__init__.py matches the actual content of the docstring. The output would of course be much longer than that. Maybe a flag to not run this test in that docstring should be included?

The weird thing is that it passes the test on the previous branch.

Nevermind, it turned out one of the example listed sites should no longer be returned because it produced a cut where not both generated sequences were double strand.

manulera added a commit to manulera/biopython that referenced this pull request Feb 27, 2024
@peterjc
Copy link
Member

peterjc commented Feb 27, 2024

I would find this easier to read if the fix and the refactoring were done as two separate commits. Would that be easy to do in principle, and is your git knowledge up to it?

(I've not looked at the change, nor quite how the refactoring works - but agree there was duplicated code)

@manulera
Copy link
Contributor Author

Hi @peterjc I splitted the change as you requested

@peterjc peterjc changed the title Issue 4604 Fix inconsistent restriction digest cutting behaviour Feb 28, 2024
@peterjc
Copy link
Member

peterjc commented Feb 28, 2024

The refactoring as a separate commit is much clearer to me - thank you!

@peterjc
Copy link
Member

peterjc commented Feb 29, 2024

Over to you @MarkusPiotrowski - this looks good to me from a style/technical point of view.

@MarkusPiotrowski
Copy link
Contributor

Yes, I'll have a look at it.

@manulera
Copy link
Contributor Author

Hi @MarkusPiotrowski just following up on the review request.

@manulera
Copy link
Contributor Author

Hi @peterjc @MarkusPiotrowski this has been pending since February, not sure if you can find someone else to review the PR. The changes are fairly small:

  • Refactor to remove code duplication: 81c0bdb
  • Feature implementation (-5 +7 lines) + tests: bb13298
  • Refactor of feature based on @peterjc review: a2d2e48

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inconsistent enzyme.search behaviour when cutting sequence falls outside sequence
3 participants