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

Example for using attrs to model config sections #242

Closed xrotwang closed 5 years ago

xrotwang commented 5 years ago

I think this bundles the information pertaining to config sections nicely. We could also easily create documentation from classes like Admin, accessing documentation and defaul values, etc.

The relevant changes are here https://github.com/lmaurits/BEASTling/pull/242/commits/e3c642d925fb3313faa564863de9ade134e8b2cb

codecov-io commented 5 years ago

Codecov Report

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

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #242    +/-   ##
======================================
  Coverage       0%     0%            
======================================
  Files          24     28     +4     
  Lines        2946   3092   +146     
======================================
- Misses       2946   3092   +146
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 21 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...c5cb7c4. Read the comment docs.

xrotwang commented 5 years ago

I think so, too. Although the python logging library is somewhat cumbersome to use, it's still the kind of use case it was designed for, so we should use it.

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

@lmaurits commented on this pull request.

In beastling/util/misc.py https://github.com/lmaurits/BEASTling/pull/242#discussion_r309879144:

@@ -0,0 +1,56 @@ +import newick + + +def sanitise_tree(tree, tree_type, languages):

Definitely! I have long been unhappy with how huge Configuration is.

But one problem with factoring stuff out into helper functions is that it becomes cumbersom to pass messages to the user. This already troubles me with our CLDF loading stuff - it's in a function by itself, and can't easily append to the calling model's messages. Probably the tree sanitizer is unlikely to want to leave log messages, but it's a pain point in general.

But then, perhaps collecting messages in a list to print at the end is a silly approach (it's not nice when things crash before the print). Maybe we need a utility log function which reads some global flag, set by the CLI, to determine whether to print the stuff it is passed or stay quiet for when BEASTling is used as a library...

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

Anaphory commented 5 years ago

I don't have much to say about this right now except that it looks like a good step forwards overall, and seems to work. I have not assessed it in detail, so this is an “Appears Good To Me”, not an “everything Looks Good To Me”, but I hope that's enough.

Anaphory commented 5 years ago

Have I really merged this myself? I don't remember doing that manually here – have we lost branch protection on master and I pushed to it by accident??

lmaurits commented 5 years ago

It's possible that I turned off master branch protection in anger one night in Jena so I could just do the damn 1.5.0 release... 😇

If we are going to drop the develop branch as suggested in #230, I feel like strict protections on master could become a bit of an impediment. But I'm happy to be schooled on what the appropriate working model is here. Every single change has to happen in its own branch of master and then be merged in after failing tests?

xrotwang commented 5 years ago

My favourite model nowadays is local, short-lived branches for all development work, which are merged into master from pull requests. The advantage of this over PRs from forks is that the others can fix PRs by also pushing to the local branch. But I guess this would make most sense if beastling had it's own github org. Giving others push access in a personal repos seems not best practice.

Luke Maurits notifications@github.com schrieb am Do., 8. Aug. 2019, 14:10:

It's possible that I turned off master branch protection in anger one night in Jena so I could just do the damn 1.5.0 release... 😇

If we are going to drop the develop branch as suggested in #230 https://github.com/lmaurits/BEASTling/issues/230, I feel like strict protections on master could become a bit of an impediment. But I'm happy to be schooled on what the appropriate working model is here. Every single change has to happen in its own branch of master and then be merged in after failing tests?

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

Anaphory commented 5 years ago

If we are going to drop the develop branch as suggested in #230, I feel like strict protections on master could become a bit of an impediment. But I'm happy to be schooled on what the appropriate working model is here.

I'm messy some of the time, I would appreciate it if I had to at least explicitly skip tests before my changes end up in master. If there is support for just removing my personal privileges to directly push to master, that's one way to do it.

Every single change has to happen in its own branch of master and then be merged in after failing tests?

… preferably after succeeding tests, not failing them ;)

@xrotwang's model sounds good.