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

Rm getters setters #7

Merged
merged 35 commits into from
Apr 18, 2016
Merged

Conversation

tomdcsmith
Copy link
Contributor

  • Most getters and setters changed to properties and property setters using decorators
  • Completed tests

…itten most tests for CTTVSomaticEvidenceString class.
…e strings json.

- Removed set_unique_association_field, identical code to add_unique_association_field, and not used in code.
- Changed so_name to property.
- Test of processgene and ProcessConsequenceTypeFileTsv
-Removed unused get_so_accessions
-most_severse_so as property
-Consequence type tests
@coveralls
Copy link

Coverage Status

Coverage increased (+8.5%) to 63.114% when pulling a692889 on tomdcsmith:rm_getters_setters into ffc114f on EBIvariation:master.

@tomdcsmith
Copy link
Contributor Author

I'm not sure why, but a couple of tests are passing when I run them locally, but when travis runs them they fail.

@coveralls
Copy link

Coverage Status

Coverage increased (+13.9%) to 68.496% when pulling aff63b5 on tomdcsmith:rm_getters_setters into ffc114f on EBIvariation:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+13.9%) to 68.496% when pulling 64e5e6e on tomdcsmith:rm_getters_setters into ffc114f on EBIvariation:master.

@@ -3,37 +3,9 @@
__author__ = 'Javier Lopez: [email protected]'


def _process_consequence_type_file_xls(snp_2_gene_file):
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this intentional? It is not related to getter/setter removal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was removed because the latest file provided by CTTV was in tsv format, so this is now possibly redundant. If they provide a file in xls format in the future this can be added back, but I think the reason this was in xls format initially was because a tsv file was put into xls format. If this is incorrect and they do provide a xls file in the future then the actual data is simple enough to just change to a tsv file, which keeps the code simpler.

@coveralls
Copy link

Coverage Status

Coverage increased (+13.9%) to 68.496% when pulling 6e435dd on tomdcsmith:rm_getters_setters into ffc114f on EBIvariation:master.

@cyenyxe cyenyxe merged commit d2d3ced into EBIvariation:master Apr 18, 2016
tomdcsmith pushed a commit to tomdcsmith/eva-cttv-pipeline that referenced this pull request May 23, 2016
Getters and setters replaced by properties
tomdcsmith pushed a commit to tomdcsmith/eva-cttv-pipeline that referenced this pull request Aug 1, 2016
Getters and setters replaced by properties
tomdcsmith pushed a commit to tomdcsmith/eva-cttv-pipeline that referenced this pull request Aug 1, 2016
Getters and setters replaced by properties
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.

3 participants