search

nicocvn / emDNA

Energy minimization software for DNA/proteins complexes by the Olson lab at Rutgers
https://nicocvn.github.io/emDNA/
5 stars 1 forks source link

Refactored DNASim library #8

Closed nicocvn closed 3 years ago

nicocvn commented 3 years ago

Summary

This PR brings a cleaned up CMake setup for the project and the DNASim library as well as re-organized dependencies. From a code base point of view functionalities have not been changed nor altered with the exception of the OpenDE collision detection feature which has been removed. Note that, this is still a work in progress (see #3) as this does not cover the emDNA part of the code base.

Closes #1.

nicocvn commented 3 years ago

@stodolli just noticing now that you are a member of this project ... GitHub suggests you as a reviewer for the PR so here it is 😄; I let you bounce it back to @rty10 if this is more appropriate.

Side note: this PR targets release/v1.0.0 because I am a bit reluctant to merge on master without running some test cases (see one of the question in #5).

stodolli commented 3 years ago

Never mind, I just realized now what must have happened with the main/master. @rty10 had an existing project in Bitbucket with a master branch so when pushing that project here with a default main, now we have both. Sorry, this is unrelated to the pull request.

nicocvn commented 3 years ago

Do we need to include .idea in the repo? I thought that was usually ignored and left up to the individual contributor? It works ok for me but just wondering what's the common practice.

Well ... yes and no. It is pretty straightforward at the moment to just re-create a CLion project each time but once you start organizing targets and setting parameters on them it will quickly be painful. IMHO it is fine to leave the CLion project in the .idea/ directory ... most people will not even notice it. We should mention somewhere in the doc that it can be open in CLion.

I see you are targeting the release branch with this PR but what workflow are we keeping for this project? One of my confusions is also, are we keeping both main and master? Are we doing away with master altogether, now that the convention is to use main as default?

Yes the main/master is a bit odd right now. I guess we can resolve that when doing the final merge of the release/v1.0.0 branch by targeting main.

In terms of workflow ... well it is a bit ad-hoc right now. I wanted to keep working on release/v1.0.0 until everything is cleaned up and more importantly tested/validated.

stodolli commented 3 years ago

Ok it all makes sense. So to the main point, I am assuming any of us 3 can approve the merge commit to the release now, is that correct? Or since you initiated it, you cannot approve it yourself @nicocvn?

nicocvn commented 3 years ago

Well I have no idea how this is setup. I asked @rty10 for admin permissions to actually take care of these things but it seems he had some issues with the user/permissions stuff (see #4). It seems I can approve it myself but I think it is better if someone else takes a look until we adjust permissions.