xrotwang commented 5 years ago

Pretty minimal work so far.

xrotwang commented 5 years ago

@lmaurits I don't know how good the test coverage is for phyltr; but what I'd do next, is switch to using pytest, get up to a really high test coverage. Should I continue with this?

xrotwang commented 5 years ago

Ah, ok, PyQt5 isn't something one can easily install. So probably, phyltr.commands.plot should import TreeStyle conditionally.

codecov-io commented 5 years ago

Codecov Report

Merging #11 into develop will increase coverage by 0.47%. The diff coverage is 100%.

@@             Coverage Diff             @@
##           develop      #11      +/-   ##
===========================================
+ Coverage    98.31%   98.79%   +0.47%     
===========================================
  Files           28       28              
  Lines         1248     1243       -5     
===========================================
+ Hits          1227     1228       +1     
+ Misses          21       15       -6

Impacted Files	Coverage Δ
src/phyltr/commands/scale.py	`100% <ø> (ø)`
src/phyltr/commands/dedupe.py	`100% <ø> (ø)`
src/phyltr/commands/cat.py	`100% <ø> (ø)`
src/phyltr/plumbing/sources.py	`96.36% <ø> (ø)`
src/phyltr/commands/prune.py	`100% <ø> (ø)`
src/phyltr/commands/grep.py	`96.87% <ø> (ø)`
src/phyltr/commands/stat.py	`100% <ø> (ø)`
src/phyltr/utils/cladeprob.py	`98.79% <ø> (ø)`
src/phyltr/commands/annotate.py	`100% <ø> (ø)`
src/phyltr/commands/consensus.py	`98.46% <ø> (ø)`
... and 18 more

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 910b0b8...b2a22cc. Read the comment docs.

lmaurits commented 5 years ago

Thanks a lot for doing this porting work! I'm really glad to see that "phyltr3" is finally possible.

I don't question for a second that you know a lot more than me about modern Python project infrastructure, so I'm sure it's actually normal nowadays - but I was initially surprised by the introduction of a src directory. I seem to recall that the last time I looked into what the broader Python community considered best practice, this was actually something people explicitly discouraged. But there was far less wide adoption of continuous/automated tooling back then.

lmaurits commented 5 years ago

Okay, apparently big happy green "everything works" content around the "merge" button doesn't actually mean that everything is happy. Travis is now, surprisingly, passing all tests for 3.4 and 3.5 but failing on 2.7 and 3.6. Seems like a pytest version requirement conflict in both cases?

xrotwang commented 5 years ago

Hm. Will have to investigate - the tests did pass on the PR, after all.

xrotwang commented 5 years ago

I guess adding a block

    extras_require={
        'dev': ['flake8', 'wheel', 'twine'],
        'test': [
            'mock',
            'pytest>=3.6',
            'pytest-mock',
            'pytest-cov',
            'coverage>=4.2',
        ],
    },

in setup.py would do the trick. I think this was only introduced in pytest-cov in the latest release 8 hours ago.

xrotwang commented 5 years ago

When it comes to python packaging, I came to rely on the judgement of Hynek Schlawack (https://hynek.me/articles/testing-packaging/) - and I experienced packaging errors which were not caught by local testing without the intermediate src directory.

lmaurits commented 5 years ago

Tests are happy now, thanks again!

lmaurits / phyltr

all tests pass on py3.5 #11

Codecov Report