althonos commented 5 years ago

Many changes in this PR:

Refactored the code a lot on the parsing side (ISA writer remains untouched, it will need more work to also integrate the ISA "enrichment" we discussed in August)
MzMLFile and ImzMLFile now use PyFilesystem internally
The local ontologies are always used instead of trying to reach the online ones, this should speedup the program startup without any drawbacks if we take the time to keep the embedded files up to date (could maybe be done with a git submodule or any other trick)
Unit tests ! Both using some files from the EBI FTP and local files generated by Tom (and they pass the test for the old metadata so the new code has the same behaviour as the old one)
Lazyness, creating a MzMLFile instance is instantaneous (pun intended) and the computations are done when the metadata property is accessed

Some backward incompatible changes which could motivate a v1.0.0:

the -o flag is now used to specify the direct output folder, mzml2isa will not create a new subfolder anymore
API changed a lot and old program will probably not work anymore (this includes mzml2isa-qt and imzml2isa-qt although minimal patches should fix that)
Changed progressbar from progressbar2 to tqdm which is more modern and well maintained

Tomnl commented 5 years ago

This is great Martin!

I am having a look at it now

Tomnl commented 5 years ago

Looks great. And looks like the unit-testing is passing on travis when tested against the old meta data, so that is reassuring.

I guess the appveyor CI should be updated so that it runs the unit-tests for Windows as well now?

Also, do we have tests for checking the CLI interface is working?

Happy for it to merge whenever you are ready, although might be worth getting the Appveyor CI to pass first.

codecov-io commented 5 years ago

Codecov Report

:exclamation: No coverage uploaded for pull request base (master@e4b1de9). Click here to learn what that means. The diff coverage is 73.79%.

@@            Coverage Diff            @@
##             master      #37   +/-   ##
=========================================
  Coverage          ?   70.54%           
=========================================
  Files             ?        8           
  Lines             ?      679           
  Branches          ?        0           
=========================================
  Hits              ?      479           
  Misses            ?      200           
  Partials          ?        0

Impacted Files	Coverage Δ
mzml2isa/__init__.py	`83.33% <100%> (ø)`
mzml2isa/isa.py	`93.45% <100%> (ø)`
mzml2isa/parsing.py	`30.76% <24.59%> (ø)`
mzml2isa/utils.py	`50.64% <29.03%> (ø)`
mzml2isa/usermeta.py	`20.48% <31.57%> (ø)`
mzml2isa/_impl.py	`53.33% <53.33%> (ø)`
mzml2isa/imzml.py	`70.58% <70.58%> (ø)`
mzml2isa/mzml.py	`92.75% <92.57%> (ø)`

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 e4b1de9...625fc62. Read the comment docs.

althonos commented 5 years ago

@Tomnl : I tried simply editing the appveyor.yml file to run the tests and install the required dependencies, but it seems there's an encoding error occuring within the tests that I can't reproduce since I don't have a Windows machine set up.

(I guess it's only a matter of changing getsyspath to getospath in some places but it's very inconvenient to do that through AppVeyor, would you mind trying on your machine ?)

Tomnl commented 5 years ago

Hey @althonos

I tested on my local Windows computer using python 3.7.0 and everything seemed to pass (I had to install microsoft visual c++ 2017 first though)

Commands:

pip install .
pip install -r  tests\requirements.txt
pip install -r ci\requirements.txt
green

Output:

Name                   Stmts   Miss  Cover   Missing
----------------------------------------------------
mzml2isa\__init__.py      12      2    83%   45-46
mzml2isa\_impl.py         15      7    53%   16-31
mzml2isa\imzml.py         17      5    71%   44-217
mzml2isa\isa.py          107      7    93%   107, 117, 120, 143, 189, 222, 245
mzml2isa\mzml.py         290     21    93%   130, 774-777, 801-802, 1009-1018, 1090-1100, 1179-1182, 1274
mzml2isa\parsing.py       78     54    31%   58, 98-153, 163-253, 271-275, 280-285, 289
mzml2isa\usermeta.py      83     66    20%   298-305, 308-318, 321-325, 328-391, 396-409, 413
mzml2isa\utils.py         77     38    51%   52-56, 79, 82, 90-126, 130-141, 150
----------------------------------------------------
TOTAL                    679    200    71%

Ran 55 tests in 297.893s

OK (passes=55)

Perhaps if we just upgrade to the latest version of python 3 in appveyor it will fix it (currently using py 3.5)

ISA-tools / mzml2isa

Refactoring of the mzML / imzML parser #37

Codecov Report