Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Basic build system, unit-testing, and Doxygen documentation. #1

Merged
merged 32 commits into from
Nov 30, 2021

Conversation

andrewcoughtrie
Copy link
Collaborator

Added a cmake build system along with googletest based unit-testing and doxygen documentation generation, only place holder source files at the moment.

@TeranIvy TeranIvy added under review Pull request being reviewed and removed ready for review ready for review labels Nov 1, 2021
Copy link
Collaborator

@TeranIvy TeranIvy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is good to see a build system put in place. I have quite a few comments and suggestions, mainly requests for additional explanations and comments. I believe that the developer knows what each file/module is about, but not everybody may be on the same page 😄

I checked out the branch and built the tool as outlined. Everything works.

I took the liberty of creating a project Changelog and adding it directly to master, so I would kindly ask the developer to merge the master to the branch after addressing code review comments so I can add the commit-to-master message to changelog.

I also added initial development and code review documentation to the wiki, see the links: https://github.com/MetOffice/profiler/wiki

Re addressing changes, I would request prefixing the commits with the PR number as outlined in the development guidance: https://github.com/MetOffice/profiler/wiki/DevelopmentTesting

.gitignore Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
tests/hello_test.cpp Outdated Show resolved Hide resolved
tests/hello_test.cpp Show resolved Hide resolved
tests/hello_test.cpp Outdated Show resolved Hide resolved
tests/hello_test.cpp Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@TeranIvy TeranIvy added reviewed with actions Changes requested and removed under review Pull request being reviewed labels Nov 5, 2021
@TeranIvy
Copy link
Collaborator

TeranIvy commented Nov 5, 2021

Back to @andrewcoughtrie to address comments.

@andrewcoughtrie
Copy link
Collaborator Author

Made the changes requested so returning to @TeranIvy for another review.

@andrewcoughtrie andrewcoughtrie added ready for review ready for review and removed reviewed with actions Changes requested labels Nov 15, 2021
@andrewcoughtrie andrewcoughtrie added ready for review ready for review and removed reviewed with actions Changes requested labels Nov 30, 2021
@andrewcoughtrie
Copy link
Collaborator Author

Made the requested changes, back to @TeranIvy for another look.

Copy link
Collaborator

@TeranIvy TeranIvy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost there. There are a couple of style remarks (see inline).

I would also request that the created build directory be added to .gitignore. According to the instructions in the README.md, build is created in the root repo directory, so I think it is safe to assume that the users will do that, too.

src/CMakeLists.txt Outdated Show resolved Hide resolved
documentation/Doxygen/Profiler.md Outdated Show resolved Hide resolved
@TeranIvy TeranIvy added reviewed with actions Changes requested and removed ready for review ready for review labels Nov 30, 2021
@andrewcoughtrie
Copy link
Collaborator Author

Added the build directory to the .gitignore file, also tidied up how it ignores the .idea directory used by my IDE, not sure why it split everything before but it just ignores the whole directory now.

@andrewcoughtrie andrewcoughtrie added ready for review ready for review and removed reviewed with actions Changes requested labels Nov 30, 2021
@andrewcoughtrie
Copy link
Collaborator Author

Back to @TeranIvy

@TeranIvy TeranIvy added ready to be merged and removed ready for review ready for review labels Nov 30, 2021
Copy link
Collaborator

@TeranIvy TeranIvy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All well now. I updated a local working copy of the branch and rebuilt and retested the code. All fine.

Updated CHANGELOG.md for the merge to master. Proceeding to merge.

@TeranIvy TeranIvy merged commit fe11fee into main Nov 30, 2021
@TeranIvy TeranIvy deleted the build-system branch November 30, 2021 17:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants