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

Just some documentation. #2541

Open
wants to merge 2 commits into
base: devel
Choose a base branch
from

Conversation

nandanvasudevan
Copy link

Description

Adding includes to the documentation and a basic test case with a lambda.

GitHub Issues

#2519 - Added headers for parts of Catch2 that I am aware of.
#2513 - Basic test case with a lambda.

@codecov
Copy link

codecov bot commented Oct 5, 2022

Codecov Report

Merging #2541 (ea37a0c) into devel (77f7c01) will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##            devel    #2541      +/-   ##
==========================================
+ Coverage   91.57%   91.58%   +0.01%     
==========================================
  Files         183      183              
  Lines        7564     7564              
==========================================
+ Hits         6926     6927       +1     
+ Misses        638      637       -1     

@nandanvasudevan
Copy link
Author

Is there anything I should do here?

@horenmar
Copy link
Member

horenmar commented Nov 5, 2022

Hi, sorry for taking so long. Your PR happens to be in the goldilocks zone of not being good enough to approve outright, not being bad enough to reject and the subject matter not being interesting enough to prioritize (small doc update versus SKIP or adding support for comparisons including std::*_ordering).

Anyway, I think the PR should be split into two, so they can progress separately. I recommend moving the lambda part into its own PR (I'll leave some notes on that separately), and leaving the include docs in this one.

}
```
[godbolt](https://catch2.godbolt.org/z/ebdr9vKcj)

Copy link
Member

Choose a reason for hiding this comment

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

This shows, but doesn't explain to non-experts, an important feature: the fact that assertions can happen outside the TEST_CASE.

There is also something interesting to be said about lambdas specifically: how do their capture lists behave across multiple invocations (due to SECTIONs).

So I would like to see a bit of text first explaining assertions in separate functions, then another piece about lambdas.

@@ -16,6 +16,9 @@ Catch is different. Because it decomposes natural C-style conditional expression
Most of these macros come in two forms:

## Natural Expressions
```cpp
#include <catch2/catch_test_macros.hpp>
```
Copy link
Member

Choose a reason for hiding this comment

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

I like this format for larger sections (ideally with their own headings) 👍

```cpp
#include <catch2/generators/catch_generators_random.hpp>
```

Copy link
Member

Choose a reason for hiding this comment

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

But I don't like it for lists, and I don't like adding new sections just to have the include paths in them.

Copy link
Member

Choose a reason for hiding this comment

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

I think there is enough extra information to be mentioned about the random generators (e.g. their repeatability guarantees) that this subheading could be filled out to be meaningful.

However, I don't see this also being true for the range generator, so we will need a different format for inline markings of things in lists.

### Range generators
```cpp
#include <catch2/generators/catch_generators_range.hpp>
```
Copy link
Member

Choose a reason for hiding this comment

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

Because the text below now reads as part of the range generators.

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.

2 participants