scitran / core

RESTful API

https://scitran.github.io

MIT License

18 stars 18 forks source link

Generic storage #1059

Open davidfarkas opened 6 years ago

davidfarkas commented 6 years ago

969 Switch to PyFilesystem

Review Checklist

Tests were added to cover all code changes
Documentation was added / updated
Code and tests follow standards in CONTRIBUTING.md

nagem commented 6 years ago

I started adding comments to the PR as I went through it even though work is still being done on the branch. Address anything I mentioned now or wait until later when we move into a more "official" review state.

codecov-io commented 6 years ago

Codecov Report

Merging #1059 into master will increase coverage by 0.06%. The diff coverage is 94.79%.

@@            Coverage Diff             @@
##           master    #1059      +/-   ##
==========================================
+ Coverage   90.98%   91.05%   +0.06%     
==========================================
  Files          49       49              
  Lines        7036     7067      +31     
==========================================
+ Hits         6402     6435      +33     
+ Misses        634      632       -2

ambrussimon commented 6 years ago

Did more thorough review. I left a couple minor comments, and there's also the merge conflict, but LGTM!

nagem commented 6 years ago

My final testing of the migration script edge cases (Analysis inputs and files changing while the upgrade is taking place) passed as expected. Awesome work!!

After a rebase I'd consider this PR approved and ready to merge, but we should hold off on merging until file-browser goes in first. That will also be after the switch over to the new repo location.