-
Notifications
You must be signed in to change notification settings - Fork 1
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
Update existing cohort parser with warning flag #616
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.
Could you resolve the linting and test failures first? Also did you do a test-run of this change?
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev #616 +/- ##
==========================================
+ Coverage 70.56% 70.64% +0.07%
==========================================
Files 116 116
Lines 9316 9337 +21
==========================================
+ Hits 6574 6596 +22
+ Misses 2742 2741 -1 ☔ View full report in Codecov by Sentry. |
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.
Awesome job! Just a few nits.
web/package-lock.json
Outdated
@@ -1,12 +1,12 @@ | |||
{ | |||
"name": "metamist", | |||
"version": "6.3.0", |
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.
Did you intend to bump version here?
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 did not, should it be 6.3.0?
…as well as length of read_filenames. Also changed removed /venv in .gitignore file - probably best for a separate PR
Co-authored-by: Vivian Bakiris <79084890+vivbak@users.noreply.github.com>
Co-authored-by: Vivian Bakiris <79084890+vivbak@users.noreply.github.com>
…th-skip-flag' into update-existing-cohort-parser-with-skip-flag
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.
Great job! We need a release for this to work. I'll action now.
Closes #614
Added warning flag to log missing data when parsing manifests as opposed to raising a
ValueError
. Also added test case that fails when aValueError
is raised and the warning flag is set toTrue
.