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

Align assertion statements; change some stderr to stdout and flush #322

Closed
wants to merge 2 commits into from

Conversation

pianistrevor
Copy link

@pianistrevor pianistrevor commented Feb 23, 2022

Highlights from CHANGELOG.md

  • See CHANGELOG.md for more

Issues Fixed

cerr << " " << (pass ? "" : "not ") << "ok " << ++mAssertCounter << " - " << description << endl;
if (!pass) {
if (pass) {
cout << " ok " << ++mAssertCounter << " - " << description << endl << flush;
Copy link
Contributor

Choose a reason for hiding this comment

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

Indentation is off here.

cerr << description << " " << lhsLabel << " " << opLabel << " " << rhsLabel << endl;
if (!pass) {
if (pass) {
cout << " ok " << ++mAssertCounter << " - ";
Copy link
Contributor

Choose a reason for hiding this comment

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

Indentation is off here.

@@ -22,6 +22,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
- Update .gitattributes so we have consistent line endings
- Change 266 files from CRLF to LF.
- Run tests on push as well as on a pull request so developers can see impact
- Align value indicators in assertion statements
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe I am being picky, but it would be great if you could split up the current commit (4bf8b53) into two separate commits, one for each entry here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you post a screenshot of the before/after alignment? I'm not sure what this change is addressing


#define assertEqualFloat(arg1, arg2, arg3) arduinoCIAssertOp("EqualFloat", "epsilon", arg3, compareMoreOrEqual, ">=", "actualDifference", fabs(arg1 - arg2))
#define assertNotEqualFloat(arg1, arg2, arg3) arduinoCIAssertOp("NotEqualFloat", "epsilon", arg3, compareLessOrEqual, "<=", "insufficientDifference", fabs(arg1 - arg2))
#define assertEqualFloat(arg1, arg2, arg3) arduinoCIAssertOp("EqualFloat", " epsilon", arg3, compareMoreOrEqual, ">=", "actualDifference", fabs(arg1 - arg2))
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should either add some comments explaining how the extra spaces work, or possibly work around it entirely by making it part of the macro.

@ianfixes
Copy link
Collaborator

Please convert tabs to spaces in your code

@ianfixes
Copy link
Collaborator

Let's delay merging this until we get more discussion on #321. I'd also like some reassurance that cout << "blah" << flush is functionally equivalent (in terms of segfaults) to cerr << "blah".

I don't mean to be pedantic about this, but the original code I wrote use cout instead of cerr and I made the switch after troubleshooting unit tests that had segfaults in them: adafruit/Adafruit-WS2801-Library#20

In other words, cerr was the difference between seeing and not seeing the source of that segfault.

@pianistrevor
Copy link
Author

I think I'll take just the indentation and apply suggested changes in a new PR, while holding off on the cerr discussion.

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.

Output errors to stderr
3 participants