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

Update catch #22

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Update catch #22

wants to merge 4 commits into from

Conversation

Herschenglime
Copy link

This pull request simply updates the version of catch provided and provides a 2 command system for building catch separately and then running your tests.

More could be done to make this easier for students to work with, but through @Brian-Magnuson and I's discussion we realized that how much extra scaffolding we provide depends on what the vision for the Edugator templates are.

As it stands now, the changes provided do not change the workflows on each programming quiz's Canvas page. It imports into Clion as expected and now benefits from quicker building without extra user input, and students will still have to open the command line from VSCode and run the commands to test, which will also benefit from speed improvements.

However, it is very possible to improve this workflow further - Brian proposes adding a Makefile so that students with make installed will only have to run make and the commandline process will work as expected. My thought is that if we're including a Makefile, we might as well include a CMakeLists.txt instead (or in addition to the makefile) to enable the same workflow that the Github templates provide. However, this would unavoidably expand the scope of what's provided in the template.

Before we make a final decision, this will be a draft PR to signal that it's not ready for merging. What are your thoughts @kapooramanpreet?

Copy link
Member

@kapooramanpreet kapooramanpreet left a comment

Choose a reason for hiding this comment

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

Good jo on the first draft. Fix readme and other stuff.

1_C++/suffix_count/build/readme.md Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Do we need a subfolder?

Copy link
Author

Choose a reason for hiding this comment

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

Not necessarily, we can remove it if you prefer

Copy link
Member

Choose a reason for hiding this comment

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

remove it then, trying to reduce files and folders.

g++ -std=c++14 -Werror -Wuninitialized -g -c test-unit/catch/catch_amalgamated.cpp -o build/catch_amalgamated.o

# then run this command to compile and run the tests (you do not need to run the previous command again)
g++ -std=c++14 -Werror -Wuninitialized -g build/catch_amalgamated.o test-unit/test.cpp -o build/test; ./build/test
Copy link
Member

Choose a reason for hiding this comment

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

this should be fixed in main readme as well.

@Brian-Magnuson
Copy link

I updated the readme, makefile, and test.cpp files.

@Brian-Magnuson Brian-Magnuson marked this pull request as ready for review February 15, 2024 18:18
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.

3 participants