-
Notifications
You must be signed in to change notification settings - Fork 28
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
Feature/compara datachecks #173
Feature/compara datachecks #173
Conversation
Pull Request Test Coverage Report for Build 870
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, good job :) Many DCs are ready to go straight in !
You may have noticed that the coverage has dropped by almost 1%. This is because you don't have unit-tests for the new methods you have added. All the utility functions should be thoroughly. Can you please add some in t/TestDataCheck.t
?
Forgot to mention it but 🥇 for documenting the new methods |
… HC CheckFirstLastRelease
Removal of incorrect line Remove whitespace
Description alteration for better wording
Edits in DataCheck.pm
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something I just spotted because I looked at the changes from the terminal. You have many lines that end with some trailing whitespace. Although we don't enforce it in Perl (we will in Python, though) it is considered better to clean that up. I don't know what @james-monkeyshines' policy is. In this instance, I would suggest to leave the PR but fix it on the lines you need to change to address the comments. On the next PRs though, make sure the trailing whitespace is removed
I am still having arguments with Atom. I hate the trailing white space, but I can't seem to get Atom to understand without then removing trailing white space when it shouldn't. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(haven't finished checking MemberProductionCounts yet)
…Attr, CheckSynteny and MultipleGenomicAlignBlockIds Following review from @muffato Reverse accidental commit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I haven't finished MemberProductionCounts and CheckFlatProteinTrees
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost done :) 😌
re-update index.json Compara for travis. Addressing more review comments from @muffato Addressing review comments & additional descriptive fixes Update lib/Bio/EnsEMBL/DataCheck/Checks/MemberProductionCounts.pm Co-Authored-By: Matthieu Muffato <muffato@ebi.ac.uk> Apply suggestions from code review Co-Authored-By: Matthieu Muffato <muffato@ebi.ac.uk> Changes following review Addressed more comments in code review Review address Review comments addressing :+1:
🙏 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
Co-Authored-By: Carla Cummins <carlac@ebi.ac.uk>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments on a few minor issues, the only one that needs to be fixed before merging is the name of the module in HighConfidence.
Addition of new critical compara DCs converted from HCs:
Addition of new method:
is_one_to_many