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

Improve display of bools #45

Merged
merged 2 commits into from
Dec 26, 2019
Merged

Improve display of bools #45

merged 2 commits into from
Dec 26, 2019

Conversation

e-kwsm
Copy link
Contributor

@e-kwsm e-kwsm commented Dec 5, 2019

Consider the following example:

#include <bandit/bandit.h>

go_bandit([] {
  bandit::describe("example", [] {
    bandit::it("false is not true", [] {
      AssertThat(false, snowhouse::Is().True());
    });
    bandit::it("true is not false", [] {
      AssertThat(true, snowhouse::Is().False());
    });
  });
});

int main(int argc, char** argv) {
  return bandit::run(argc, argv);
}

Obviously the tests fail but messages are:

FF
There were failures!

example false is not true:
a.cpp:6: Expected: true
Actual: 0


example true is not false:
a.cpp:9: Expected: false
Actual: 1

Test run complete. 2 tests run. 0 succeeded. 2 failed.

true and false are respectively printed as 1 and 0.

To fix this, std::boolalpha is added.

@sbeyer
Copy link
Member

sbeyer commented Dec 5, 2019

Thank you very much! I acknowledge the issue. However, I have to check if I like this solution (I think those C++ I/O manipulators have an ugly and slightly mystical touch). Moreover, I would like to see a test case for this scenario.

@sbeyer
Copy link
Member

sbeyer commented Dec 5, 2019

I have to check if I like this solution (I think those C++ I/O manipulators have an ugly and slightly mystical touch).

I still have not found consensus with myself.
Although it does not hurt in any way, it feels wrong to set the boolalpha flag even if no bool is going to follow.

Hence I somehow tend to prefer the template specialization solution, like (edit note: totally untested)

  template<>
  struct Stringizer<bool>
  {
    static std::string ToString(bool value)
    {
      return value ? "true" : "false";
    }
  };

although it is much more code.

@sbeyer
Copy link
Member

sbeyer commented Dec 5, 2019

There's a third point (first was test cases, second the question whether template specialization is possibly "better" than boolalpha): the commit message. (GitHub does not allow to comment on commit messages directly.)
"Add boolalpha" gives no real information to the reader. It's probably better to have a subject like "Stringize bools to true/false (not 1/0)" and use the body of the commit message to say how this is accomplished.

Sorry for the nitpicking here. But I feel it is often forgotten that commit messages are also "first-class citizens" of a repository, not only the code itself.

  1. add test cases (one for true, one for false),

@e-kwsm
Copy link
Contributor Author

e-kwsm commented Dec 9, 2019

There's a third point (first was test cases, second the question whether template specialization is possibly "better" than boolalpha): the commit message. (GitHub does not allow to comment on commit messages directly.)
"Add boolalpha" gives no real information to the reader. It's probably better to have a subject like "Stringize bools to true/false (not 1/0)" and use the body of the commit message to say how this is accomplished.

I updated the commit message.

I still have not found consensus with myself.
Although it does not hurt in any way, it feels wrong to set the boolalpha flag even if no bool is going to follow.

Hence I somehow tend to prefer the template specialization solution, like (edit note: totally untested)

  template<>
  struct Stringizer<bool>
  {
    static std::string ToString(bool value)
    {
      return value ? "true" : "false";
    }
  };

although it is much more code.

IIUC std::boolalpha has no effect on non-bool variables.

Or, how about specializing detail::DefaultStringizer?
The above code disallows users to specialize Stringizer for customization.

@sbeyer
Copy link
Member

sbeyer commented Dec 9, 2019

I updated the commit message.

Thank you.

IIUC std::boolalpha has no effect on non-bool variables.

That's right, but it still feels wrong in some way... (Does it, really? 🤔🤔🤔)

Or, how about specializing detail::DefaultStringizer?

Yes, thanks! I was too lazy looking up its name :) I think that's the way I'd prefer it.

@e-kwsm
Copy link
Contributor Author

e-kwsm commented Dec 10, 2019

detail::DefaultStringizer was specialized.

@sbeyer
Copy link
Member

sbeyer commented Dec 10, 2019

Lovely, thank you very much! Will you write test cases or do I have to? :)

PS: I wonder why the GitHub Actions for "push" are not triggered.

@e-kwsm
Copy link
Contributor Author

e-kwsm commented Dec 11, 2019

Lovely, thank you very much! Will you write test cases or do I have to? :)

I would appreciate if you write it.

PS: I wonder why the GitHub Actions for "push" are not triggered.

pull_request is required.

@sbeyer
Copy link
Member

sbeyer commented Dec 11, 2019

I would appreciate if you write it.

Ok, thanks for your work!

pull_request is required.

Thanks for adding. But what I mean is this: you have pushed to e-kwsm/snowhouse but your push has not triggered the action. 🤔

edit: Your last push triggered the action, but your previous pushes did not. Weird.

@e-kwsm
Copy link
Contributor Author

e-kwsm commented Dec 11, 2019

First I activated (required?) GitHub Actions on my fork and forcely pushed c765187 to check it, and GitHub actions were triggered (I hope this link is accessible).
Then I thought you meant GitHub Actions on this repo, so pushed a17f53e.

I'm not sure why but it seems that other people cannot access to https://github.com/e-kwsm/snowhouse/actions. Actually it is possible to access via history. 😕

@sbeyer
Copy link
Member

sbeyer commented Dec 11, 2019

First I activated (required?) GitHub Actions on my fork and forcely pushed c765187 to check it, and GitHub actions were triggered

Ah, so you had to activate GitHub Actions on your fork. I did not know that this is necessary; this explains my confusion. Thank you for shedding light on this. I originally thought that this is always activated, and that's why I did not initially use pull_request for my workflows; I did not see any sense to let these actions run twice (once for the push, once for the PR).

(I hope this link is accessible)

It is.

@sbeyer sbeyer changed the base branch from master to better-default-stringizers December 26, 2019 22:31
@sbeyer sbeyer changed the title Add std::boolalpha Improve display of bools Dec 26, 2019
@sbeyer sbeyer merged commit 79400c8 into banditcpp:better-default-stringizers Dec 26, 2019
@e-kwsm e-kwsm deleted the boolalpha branch December 26, 2019 22:59
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