Add qiimp xlsx

[antgonza]

No description provided.

[codecov-io]

Codecov Report

Merging #2599 into release-candidate will increase coverage by 0.01%.
The diff coverage is 100%.

@@                  Coverage Diff                  @@
##           release-candidate    #2599      +/-   ##
=====================================================
+ Coverage              94.44%   94.46%   +0.01%     
=====================================================
  Files                    160      160              
  Lines                  19523    19568      +45     
=====================================================
+ Hits                   18439    18484      +45     
  Misses                  1084     1084

Impacted Files	Coverage Δ
qiita_db/metadata_template/test/test_util.py	99.57% <100%> (+0.03%)	⬆️
qiita_db/metadata_template/util.py	91.21% <100%> (+0.12%)	⬆️
qiita_db/util.py	96.81% <100%> (+0.12%)	⬆️

---

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 679528b...39d7906. Read the comment docs.

[ElDeveloper]

Just one comment regarding the location of the changes.

[ElDeveloper]

This is going to fail when filepath_or is not a string because it won't have the endswith method, right?

I think the XLSX parsing should be moved elsewhere this function seems too generic as it should only open a file path or file handle. Is there any motivation to have this here instead of in the sample/prep information parsing?

[antgonza]

Right! I initially was gonna add the code in load_template_to_dataframe but realized that it really just calls this function (see, https://github.com/biocore/qiita/blob/master/qiita_db/metadata_template/util.py#L100) as it just calls the open_file method. BTW note that the same method is available to open hdf5, text, or iostring. Thus, I believe is fine to fail if not a string as the other code should be able to handle and parse correctly those errors. Thoughts?

[ElDeveloper]

Fair enough, the use of this function is rather limited (see these search results). It still seems too odd to me that a function that's intended to return a file handle will parse a file write it as a txt file and return that file handle.

[antgonza]

Well, the idea of parsing the xmlx is that it will be transformed to txt and then use the regular prep/sample code ... ¯_(ツ)_/¯ ... I don't have a better place to move it but if you do, let me know.

[ElDeveloper]

I think it makes more sense to have this outside of this function. I see as two very different things, (1) opening a file and (2) parsing a file. And this function opens the file and shouldn't be parsing it. Is there any technical limitation to putting this in load_template_to_dataframe? Perhaps as a helper function somewhere in there?

[AmandaBirmingham]

This isn't a show-stopper, but I am concerned because I think the auto-generated exception is inaccurate ... an empty QIIMP template will contain a sample_name column, as shown below, and the tsv produced from it will also include that column--actually, the tsv will contain ONLY a row of column names :) I fear that an error message indicating the sample_name column isn't there when it actually is will mislead users/future maintainers when they try to figure out why their upload failed.

[antgonza]

@AmandaBirmingham, good point. The reason this was happening was all columns are empty so all of them are dropped and the final dataframe was empty, and the first test is to check for validity is if sample_name column is there. Anyway, I added a new tests (empty) and now the error should make more sense.