shendurelab / LACHESIS

The LACHESIS software, as described in Nature Biotechnology (http://dx.doi.org/10.1038/nbt.2727)
Other
75 stars 33 forks source link

potential pull request #25

Open abelew opened 7 years ago

abelew commented 7 years ago

Greetings, Some time ago (September?) I was playing with Lachesis and made a fairly large number of trivial changes suggested by the clang tidy tool; I promptly forgot about them. This morning, someone in the department asked for a little help using Lachesis and I found myself making a few new changes (notably changing the C style comments in the shell/R/perl scripts) so that the various scripts will work for him.

I do not wish to make a big pull request without first checking in. Thus, if you would consider it, perhaps take a peek at: https://github.com/abelew/LACHESIS/commits/master and note the commits made in September. If they are not too troublesome, I would be happy to send a pull request (which at least as of now does not conflict with anything).

Probably the best change there is a Makefile target which calls upon the clang-tidy tool.

I hope you have a good day, atb

JingaJenga commented 7 years ago

Hi Ashton,

Thanks so much for your efforts to improve LACHESIS! As you may have noticed, my active development of the code base has stopped. But I certainly don't mind merging in other people's contributions, and I appreciate the effort you've taken!

Your last pull request in September was fine (though it caused a minor conflict that I had to sort out - not your fault!) Please send me a pull request for your latest changes and I'll look them over and probably merge them in.

Thanks again, -- Josh

abelew commented 7 years ago

Hi, OK, will do. I did some more fiddling over the weekend and partially autoconfiscated the project. I say partially because it has been 15 years since I set up autoconf/automake on a project which did not already have it -- as a result I messed up the CFLAGS such that when linking it yells at me that there are multiple function definitions for most of the functions in RunParams, TestFileParsers, ClusterVec, ContigOrdering, etc. Ergo I think that I messed up the autoconf creation of CFLAGS. In the worst case scenario, my previous changes were actually messed up and they did not actually compile and I somehow did not notice. This I think is less likely, as I think some folks are using it on our cluster and expressed happiness to me that it no longer runs out the stack on the machine; and that the R/perl/shell scripts were no longer failing.

Thus perhaps it would be unwise to merge in my changes without some further testing.

If you are interested, I can commit the autoconf changes with the caveat that they are still broken, but work right up until linking and probably only require a modification of Makefile.am (AM_* I bet, as automake macros are confusing to me) and a minor reorganization of the include/ directory to follow the gnu conventions. Assuming I get a chance this week I will try that and see what happens.

Have a nice day! atb

On 01/16/2017 11:39 AM, Josh Burton wrote:

Hi Ashton,

Thanks so much for your efforts to improve LACHESIS! As you may have noticed, my active development of the code base has stopped. But I certainly don't mind merging in other people's contributions, and I appreciate the effort you've taken!

Your last pull request in September was fine (though it caused a minor conflict that I had to sort out - not your fault!) Please send me a pull request for your latest changes and I'll look them over and probably merge them in.

Thanks again, -- Josh

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/shendurelab/LACHESIS/issues/25#issuecomment-272909910, or mute the thread https://github.com/notifications/unsubscribe-auth/AAEA7VyF1HhhyRacI02JVeLfsGy9_CK2ks5rS51KgaJpZM4LjFdF.

JingaJenga commented 7 years ago

Hi Ashton,

I would say we should wait until the fixes are done before committing them. There's no rush. Just let me know when they're done and send a pull request, and I'll merge it.

-- Josh