Skip to content

Conversation

@andersonjpark
Copy link

No description provided.

@srichers
Copy link
Owner

srichers commented Apr 1, 2020

Thanks, Andy! A few comments to be addressed before I pull. Learning to collaborate on git is part of the process, so I am happy that you are sending pull requests that require some fixing - the quicker we make mistakes the quicker we learn :-)

I think most of this would be fixed by following the workflow that I mentioned, i.e. to never make blanket commits, since many of the changes that need to be reversed would have been almost impossible by committing individual files. Since the code is known to work, it is especially important to make only absolute minimal changes to accomplish a particular objective (i.e. Makefile_Andy and the single file where you added the new function).

(1) By removing the "external" directory, you removed the NuLib submodule from the git repository. It is fine to check out and use NuLib in a separate directory, but we should still leave NuLib as a submodule (right now you have deleted the submodule).

(2) You committed changes to Makefile_Andy, which is perfect. However, you also committed a Makefile, which means that whenever I try to pull it will overwrite my makefile with your version.

(3) I appreciate that whitespace was committed in a separate commit!

(4) nulib_make.inc should not be deleted

(5) Binary files should never be added (e.g., the sqa executable and the downloaded HDF5 tarball)

(6) external should not be included in the gitignore - needed to sense changes to the NuLib submodule

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants