-
Notifications
You must be signed in to change notification settings - Fork 79
Sample generate files improvements #2749
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
Sample generate files improvements #2749
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.
Request response to comments, before approving; changes not needed if you feel that it is fine.
"DB and the new data provided", | ||
qdb.exceptions.QiitaDBWarning) | ||
return | ||
return set([]), set([]) |
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.
Is it better to return two empty sets vs None, or (None, None), throw an Exception, etc?
md_template, self.study_id, current_columns=self.categories()) | ||
self._update(new_map) | ||
samples, columns = self._update(new_map) | ||
self.validate(self.columns_restrictions) |
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.
Feels like it'd be more readable to test for not None or similar here, instead of having to go down into validate(), but I defer.
Codecov Report
@@ Coverage Diff @@
## dev #2749 +/- ##
=========================================
+ Coverage 89.62% 94.3% +4.68%
=========================================
Files 93 166 +73
Lines 6459 19980 +13521
=========================================
+ Hits 5789 18843 +13054
- Misses 670 1137 +467
Continue to review full report at Codecov.
|
#2743 should be reviewed/merged first