-
Notifications
You must be signed in to change notification settings - Fork 12
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
Fix build and tests #98
Conversation
Google downloads seem to work now anyway, given up to date urls retrieved from google drive. The lesson to be learned here is use libraries rather than fragile string parsing. To that end, we should probably use an actual Google drive client rather than the hack I've got working for now Also stop running tests twice, which has been happening for 4 years apparently
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #98 +/- ##
==========================================
+ Coverage 70.89% 71.53% +0.63%
==========================================
Files 5 5
Lines 1182 1212 +30
==========================================
+ Hits 838 867 +29
- Misses 344 345 +1 ☔ View full report in Codecov by Sentry. |
The coverage report seems to have multiple prior PRs included, as there are changes not included in this PR. For some reason this repo was deactivated in codecov |
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. Just need to bump version and add release note.
@@ -11,6 +11,7 @@ | |||
import subprocess |
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.
We need to bump version as current release version is also 0.2.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.
Yep, coming up next
parsedurl = urlparse.urlparse(file_url) | ||
query = {} | ||
if parsedurl.query: | ||
# should we catch an error 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 don't have a strong option. Probably fine as is.
Google downloads seem to work now anyway, given up to date urls retrieved from google drive. The lesson to be learned here is use libraries rather than fragile string parsing. To that end, we should probably use an actual Google drive client rather than the hack I've got working for now
Also stop running tests twice, which has been happening for 4 years apparently