-
Notifications
You must be signed in to change notification settings - Fork 507
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
lengthen sample stable id field #9404
lengthen sample stable id field #9404
Conversation
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.
Looks good to me
@@ -2667,6 +2670,14 @@ def checkLine(self, data): | |||
'column_number': col_index + 1, | |||
'cause': value}) | |||
continue | |||
if len(value.strip()) > MAX_SAMPLE_STABLE_ID_LENGTH: |
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.
Thank you! We've run into this a few times.
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.
LGTM, thanks!
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.
There seems to be a test failing: https://github.com/cBioPortal/cbioportal/runs/5648963463?check_suite_focus=true. Looks like we missed updating the db.version in db-scripts/src/main/resources/cgds.sql
?
Making sure all db versions are the same in cgds.sql, pom.xml and migration.sql
pom.xml db version is 2.12.13
db-scripts/src/main/resources/cgds.sql db version is 2.12.12
db-scripts/src/main/resources/migration.sql db version is 2.12.13
db versions mismatch
@sheridancbio I just fixed my comment ☝️ |
a3b604b
to
12019b0
Compare
- sample stable ids now can be up to 63 characters - add check for maximum stable id to validator - migration script update (schema 2.12.13) - remove wildcard from search for cgds.sql during unit tests
Excellent ... I'm so glad that test was added! .. thanks @inodb |
Kudos, SonarCloud Quality Gate passed! |
@inodb I have completed my system testing of the study validator. I used the study luad_tcga_pub from datahub. I changed one of the study ids to a length 63 string throughout the datafiles and the study successfully validated. Then I made that sample id length 64 and validation failed with this message in the logs: DEBUG: data_clinical_sample.txt: Starting validation of file I was unable to get the unit tests for the validator script to run smoothly in my environment, so I did not add a unit test for this case into for example core/src/test/scripts/unit_tests_validate_data.py or core/src/test/scripts/system_tests_validate_data.py But I think we can move forward with this now without the validator unit tests (or, if anyone can give guidance on running the unit tests, I'd be happy to put it in) |
@sheridancbio If this is the error you get when run
Ramya gave me this suggestion before, and it worked great for me: |
Describe changes proposed in this pull request:
Checks
Any screenshots or GIFs?
If this is a new visual feature please add a before/after screenshot or gif
here with e.g. Giphy CAPTURE or Peek
Notify reviewers
Read our Pull request merging
policy. It can help to figure out who worked on the
file before you. Please use
git blame <filename>
to determine thatand notify them either through slack or by assigning them as a reviewer on the PR