search

lmaurits / BEASTling

A linguistics-focussed command line tool for generating BEAST XML files.
BSD 2-Clause "Simplified" License
20 stars 6 forks source link

Major refactoring #240

Closed xrotwang closed 5 years ago

xrotwang commented 5 years ago
xrotwang commented 5 years ago

I admit that this is rather big and should not be the norm for PRs. I also made the mistake of bundling py2 removal with xml creation refactoring. But we have decent test coverage by now, so I'm fairly convinced that I didn't break anything. I also like the xml creation now, in particular because turning xml tags into python functions makes such a difference when using an IDE like PyCharm.

codecov-io commented 5 years ago

Codecov Report

Merging #240 into master will not change coverage. The diff coverage is 0%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #240   +/-   ##
=====================================
  Coverage       0%     0%           
=====================================
  Files          24     26    +2     
  Lines        2946   2956   +10     
=====================================
- Misses       2946   2956   +10
Impacted Files Coverage Δ
beastling/__main__.py 0% <ø> (ø) :arrow_up:
beastling/report.py 0% <ø> (ø) :arrow_up:
beastling/cli.py 0% <ø> (ø) :arrow_up:
beastling/clocks/relaxed.py 0% <0%> (ø) :arrow_up:
beastling/fileio/datareaders.py 0% <0%> (ø) :arrow_up:
beastling/clocks/random.py 0% <0%> (ø) :arrow_up:
beastling/models/covarion.py 0% <0%> (ø) :arrow_up:
beastling/treepriors/base.py 0% <0%> (ø) :arrow_up:
beastling/distributions.py 0% <0%> (ø) :arrow_up:
beastling/extractor.py 0% <0%> (ø) :arrow_up:
... and 17 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 ed7483c...1b958f3. Read the comment docs.

codecov-io commented 5 years ago

Codecov Report

Merging #240 into master will not change coverage. The diff coverage is 0%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #240   +/-   ##
=====================================
  Coverage       0%     0%           
=====================================
  Files          24     26    +2     
  Lines        2946   2956   +10     
=====================================
- Misses       2946   2956   +10
Impacted Files Coverage Δ
beastling/__main__.py 0% <ø> (ø) :arrow_up:
beastling/report.py 0% <ø> (ø) :arrow_up:
beastling/cli.py 0% <ø> (ø) :arrow_up:
beastling/clocks/relaxed.py 0% <0%> (ø) :arrow_up:
beastling/fileio/datareaders.py 0% <0%> (ø) :arrow_up:
beastling/clocks/random.py 0% <0%> (ø) :arrow_up:
beastling/models/covarion.py 0% <0%> (ø) :arrow_up:
beastling/treepriors/base.py 0% <0%> (ø) :arrow_up:
beastling/distributions.py 0% <0%> (ø) :arrow_up:
beastling/extractor.py 0% <0%> (ø) :arrow_up:
... and 17 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 ed7483c...1b958f3. Read the comment docs.

xrotwang commented 5 years ago

It's low complexity now, but once the xml creation goes through this central point, one could do more. Switch to a different xml backend (if needed), validate input, e.g. plates always must have var and range, ...

Luke Maurits notifications@github.com schrieb am Do., 1. Aug. 2019, 22:23:

@lmaurits approved this pull request.

This definitely looks like an improvement to me.

It's a good idea to make it as easy as possible for people who author interesting new BEAST classes (like tree priors) to also write BEASTling classes for it. So getting this kind of much neater and simpler code for functionality is IMHO well worth the cost of the quite low complexity of something like utils/xml.py.

@Anaphory https://github.com/Anaphory, what do you think?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/lmaurits/BEASTling/pull/240?email_source=notifications&email_token=AAGUOKDBOJ2VMXSD7JW255DQCNA35A5CNFSM4IIOAVC2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCAK3LZQ#pullrequestreview-269858278, or mute the thread https://github.com/notifications/unsubscribe-auth/AAGUOKHAZE7YZTSTF3AQC5TQCNA35ANCNFSM4IIOAVCQ .

xrotwang commented 5 years ago

Not yet. But the way is paved now. Waiting for this to be merged 😀

Gereon Kaiping notifications@github.com schrieb am Di., 6. Aug. 2019, 14:24:

@Anaphory commented on this pull request.

In setup.py https://github.com/lmaurits/BEASTling/pull/240#discussion_r311032442:

 install_requires=[

'newick>=0.6.0', 'appdirs',

  • 'csvw', 'clldutils>=2.8',

Shouldn't the clldutils dependency be removed now?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/lmaurits/BEASTling/pull/240?email_source=notifications&email_token=AAGUOKFXJI4I2JEJ2OAYEBTQDFUQPA5CNFSM4IIOAVC2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCAV5A2Y#pullrequestreview-271306859, or mute the thread https://github.com/notifications/unsubscribe-auth/AAGUOKHYTIBN6UK43LOH7Z3QDFUQPANCNFSM4IIOAVCQ .

lmaurits commented 5 years ago

Sorry for the delay in merging this, I didn't want to do it until @Anaphory had had a chance to look over it and give his approval. Happy it's done now!