saketkc / pysradb

Package for fetching metadata and downloading data from SRA/ENA/GEO
https://saketkc.github.io/pysradb
BSD 3-Clause "New" or "Revised" License
311 stars 51 forks source link

Issue #26 test cases and uri code updated #31

Closed DaasDaham closed 4 years ago

DaasDaham commented 4 years ago

Tested with all existing test cases (all passed) and added new test cases. List of changes: 1) Call _verify_srametadb(sqlite_file) function before calling super() to ensure first that the file is right since this function also calls BASEdb. 2) Changed sqlite.connect() to use uri syntax which can be used to control details of the newly created database connection. 3) Added a check to replace all the '?' characters in input as they were causing the input to be truncated at the first '?' character, this might be happening because the uri syntax also uses ? to start mode declaration.

saketkc commented 4 years ago

Thanks @DaasDaham for your contribution! I have left inline reviews. _test_sradb.py would not run currently. It needs to be moved to test_sradb.py if it is to be detected by pytest. The problem however with that is the SRAmetadb.sqlite.gz can sometimes take a long time to be decompressed and hence the tests often fail on travis. I haven't tested it for a while, so its definitely worth a shot.

DaasDaham commented 4 years ago

Thanks @DaasDaham for your contribution! I have left inline reviews. _test_sradb.py would not run currently. It needs to be moved to test_sradb.py if it is to be detected by pytest. The problem however with that is the SRAmetadb.sqlite.gz can sometimes take a long time to be decompressed and hence the tests often fail on travis. I haven't tested it for a while, so its definitely worth a shot.

Yes travis is throwing no space left error while extracting SRAmetadb.sqlite.gz. Can this be resolved?

saketkc commented 4 years ago

Hi @DaasDaham, I cannot think of an alternate to test the SRAmetadb mode. For now I would suggest, that we move back test_sradb.py to _test_sradb.py and add a new test_sradb.py with the only test being the one which throws an error if the file is either not present or not a valid sqlite file.

Thanks once again for your contribution!

DaasDaham commented 4 years ago

Hi @saketkc I have made all the required changes. Please have a look

DaasDaham commented 4 years ago

Hi @DaasDaham, I cannot think of an alternate to test the SRAmetadb mode. For now I would suggest, that we move back test_sradb.py to _test_sradb.py and add a new test_sradb.py with the only test being the one which throws an error if the file is either not present or not a valid sqlite file.

Thanks once again for your contribution!

Hi @saketkc, I haven't heard from you regarding this issue, I have made the changes that you mentioned in my latest commit. Please have a look at it.

saketkc commented 4 years ago

@DaasDaham can you run black . and recommit? The tests are failing because of the formatting.

codecov[bot] commented 4 years ago

Codecov Report

Merging #31 into master will increase coverage by 14.84%. The diff coverage is 59.62%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master      #31       +/-   ##
===========================================
+ Coverage   24.17%   39.02%   +14.84%     
===========================================
  Files           8        5        -3     
  Lines        1187      984      -203     
===========================================
+ Hits          287      384       +97     
+ Misses        900      600      -300     
Impacted Files Coverage Δ
pysradb/cli.py 0.00% <0.00%> (ø)
pysradb/download.py 22.41% <28.00%> (+4.63%) :arrow_up:
pysradb/basedb.py 41.86% <50.00%> (-55.76%) :arrow_down:
pysradb/sraweb.py 81.66% <81.75%> (ø)
pysradb/__init__.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 99f738e...783e017. Read the comment docs.

DaasDaham commented 4 years ago

@DaasDaham can you run black . and recommit? The tests are failing because of the formatting.

Thanks for the help! , it builds successfully now

saketkc commented 4 years ago

Thanks for your contribution @DaasDaham! Are you okay with your name being added in CONTRIBUTORS?

DaasDaham commented 4 years ago

Thanks for your contribution @DaasDaham! Are you okay with your name being added in CONTRIBUTORS?

Sure, thanks!. Just one thing, this latest commit introduced a change in the print statement on line 46 in cli.py. I am really sorry for this, I have opened a pull request to revert it back to original print statement #33

saketkc commented 4 years ago

Hi @DaasDaham, are you okay with me adding you to the AUTHORS list?