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: fix & refactor miRNA extension script #144

Merged
merged 18 commits into from
Jul 31, 2024

Conversation

uniqueg
Copy link
Member

@uniqueg uniqueg commented May 19, 2024

I had a bit of time and felt like coding, so I refactored the script a bit (well, maybe a bit more than a bit).

It should now do what it's supposed to do, I hope. Or in any case, it's broken down into more, simpler steps, so if it isn't fully there yet (the tests would show), then it should be easy enough to change.

I suppose you could merge this in your branch, if you like, and then update the tests to match and cover all edge cases. Note that I have included two or three specific error checks that were not covered before, so apart from refactoring, you'd probably need to add a couple more tests to check for these.

@uniqueg uniqueg requested a review from deliaBlue May 19, 2024 18:45
@uniqueg
Copy link
Member Author

uniqueg commented May 19, 2024

Of course you can also update/add the tests first, in this branch, so that the tests pass. Probably makes more sense :)

Copy link
Collaborator

@deliaBlue deliaBlue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here are some small comments; besides the last one, everything is related to the docstrings .
In the meantime, if it's alright, I'll be working on fixing the unit tests and static code analyses for this PR.

scripts/mirna_extension.py Outdated Show resolved Hide resolved
scripts/mirna_extension.py Outdated Show resolved Hide resolved
scripts/mirna_extension.py Outdated Show resolved Hide resolved
scripts/mirna_extension.py Show resolved Hide resolved
@uniqueg
Copy link
Member Author

uniqueg commented May 24, 2024

Here are some small comments; besides the last one, everything is related to the docstrings .
In the meantime, if it's alright, I'll be working on fixing the unit tests and static code analyses for this PR.

Awesome, thanks a lot :)

Will you address these points when you are adding the tests etc? 🙏

@deliaBlue
Copy link
Collaborator

Here are some small comments; besides the last one, everything is related to the docstrings .
In the meantime, if it's alright, I'll be working on fixing the unit tests and static code analyses for this PR.

Awesome, thanks a lot :)

Will you address these points when you are adding the tests etc? 🙏

On it! The unit tests will take me the rest of today, luckily not that much of tomorrow.

Just a small comment. The following line has more than 80 char therefore, it fails the static code analysis:

precursor.attributes["Name"][0] += f"_-{pre_orig_start - precursor.start}"

black suggests to apply the following style:

precursor.attributes["Name"][
        0
    ] += f"_-{pre_orig_start - precursor.start}"

I find it horrible. So I want to know which one of the following approach you want me to follow:

  1. Create the suffix variable, blending both formatted strings to afterwards modify the precursor name.
  2. Change the precursor name to something shorter.
  3. Change the pre_orig_start name to something shorter.
  4. Use black's suggestion.

I'd rather go with the first one but it's up to you :)

@deliaBlue
Copy link
Collaborator

deliaBlue commented Jul 7, 2024

The changes applied in the last push are:

  • Update the unit test and the required files for the script 'mirna_extension.py'
  • Refactor the 'mirna_extension.py' script
  • Update expected output (only the extended GFF3 files change) to match the new script (see this comment for more information on the changes)
  • Two new errors have been add to ignore (W0719 (broad-exception-raised) in order to use rise Exception and E0606 (possibly-used-before-assignment) as an error is raised prior to using that variable in the script 'mirna_quantification.py' lines 260, 304 and 330)

@uniqueg I cannot put you as a reviewer as you created this PR. But please, have a look. I'll wait for your feedback :))

@deliaBlue deliaBlue self-requested a review July 7, 2024 20:30
@deliaBlue deliaBlue self-assigned this Jul 7, 2024
@deliaBlue deliaBlue added the bug Something isn't working label Jul 7, 2024
@uniqueg
Copy link
Member Author

uniqueg commented Jul 22, 2024

Here are some small comments; besides the last one, everything is related to the docstrings .
In the meantime, if it's alright, I'll be working on fixing the unit tests and static code analyses for this PR.

Awesome, thanks a lot :)
Will you address these points when you are adding the tests etc? 🙏

On it! The unit tests will take me the rest of today, luckily not that much of tomorrow.

Just a small comment. The following line has more than 80 char therefore, it fails the static code analysis:

precursor.attributes["Name"][0] += f"_-{pre_orig_start - precursor.start}"

black suggests to apply the following style:

precursor.attributes["Name"][
        0
    ] += f"_-{pre_orig_start - precursor.start}"

I find it horrible. So I want to know which one of the following approach you want me to follow:

  1. Create the suffix variable, blending both formatted strings to afterwards modify the precursor name.
  2. Change the precursor name to something shorter.
  3. Change the pre_orig_start name to something shorter.
  4. Use black's suggestion.

I'd rather go with the first one but it's up to you :)

Either way is fine by me :)

Copy link
Member Author

@uniqueg uniqueg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few comments...

scripts/mirna_extension.py Outdated Show resolved Hide resolved
pylint.cfg Outdated Show resolved Hide resolved
pylint.cfg Outdated Show resolved Hide resolved
pylint.cfg Outdated Show resolved Hide resolved
scripts/tests/files/extreme_chr_mir_anno.gff3 Show resolved Hide resolved
@deliaBlue
Copy link
Collaborator

deliaBlue commented Jul 28, 2024

@uniqueg the pushed changes are:

  • Create custom exception to accurately catch and raise annotation errors
  • Account for possible illegal coordinates in the user-provided GFF3 file (add try-except block + one unit test + one unit test file)
  • Remove added errors to ignore in pylint.cfg
  • fix minor static code analysis errors in mirna_quantification.py and in validation_fasta.py

@uniqueg
Copy link
Member Author

uniqueg commented Jul 31, 2024

LGTM :)

@deliaBlue deliaBlue merged commit 42b79c2 into test-fix-static-code-analysis Jul 31, 2024
4 checks passed
@deliaBlue deliaBlue deleted the fix_mir_extension_script branch July 31, 2024 12:00
deliaBlue added a commit that referenced this pull request Jul 31, 2024
* fix: fix & refactor miRNA extension script (#144)

* fix: fix & refactor miRNA extension script

* feat: add name modification method

* test: update 'test_mirna_extension.py

* ci: update expected output

* refactor: add custom exception

* test: add unit test for illegal pos

* test: add illegal coords test file

* test: fix for static code analysis

* Update mirna_quantification.py

---------

Co-authored-by: deliaBlue <[email protected]>

---------

Co-authored-by: Alex Kanitz <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants