search

approvals / ApprovalTests.cpp

Native ApprovalTests for C++ on Linux, Mac and Windows
https://approvaltestscpp.readthedocs.io/en/latest/
Apache License 2.0
318 stars 51 forks source link

More ReSharper Fixes #32

Closed alastairUK closed 5 years ago

alastairUK commented 5 years ago

I applied a load more Resharper C++ fixes. I don't think (m)any are overly controversial

claremacrae commented 5 years ago

I've read through and they all make sense to me.

I do need to read up on when to use const std::string& in args, and when to use std::string with std::move(arg)....

claremacrae commented 5 years ago

Thank you!

One small thing for future reference, can you please prefix your commit messages with Arlo's Commit Notation (for all all these, it would be a lower case 'r ')

https://github.com/RefactoringCombos/ArlosCommitNotation

(We use these letters to help us write the release notes for each release)

Thanks again!

claremacrae commented 5 years ago

For the code that shows up in our tutorials, we are preferring less-correct, but simpler code.

The easy to see that code is being used in documentation is that it will have a snippet comment around it.

// begin-snippet: snippet_name
.... code
// end-snippet

We've undone a couple of changes in this commit:

https://github.com/approvals/ApprovalTests.cpp/commit/fb0db5a156d293beb02230f751cbadc8460febfd

If you were so inclined, you could also detect this by running this script in the top-level of the project:

 ./run_markdown_templates.sh

But it needs Cygwin to be installed and modified on windows, so we would totally understand if you didn't do this!

alastairUK commented 5 years ago

No problem- never seen that notation before. Interesting idea.

Would have happily rebased these commits to change the messages. Too late now they are merged.

Been reading up on github actions whilst lounging in the sun. Have applied for a beta access. Think an action for applying clang-format on pull requests would be good. Also the CI/CD stuff looks good (for running latest gcc/clang/vs)

On Mon, 2 Sep 2019 at 19:20, Clare Macrae notifications@github.com wrote:

Thank you!

One small thing for future reference, can you please prefix your commit messages with Arlo's Commit Notation (for all all these, it would be a lower case 'r ')

https://github.com/RefactoringCombos/ArlosCommitNotation

(We use these letters to help us write the release notes for each release)

Thanks again!

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/approvals/ApprovalTests.cpp/pull/32?email_source=notifications&email_token=ABIXFVWZJEJTFAGVDR4BJG3QHVDPNA5CNFSM4IS6NN3KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD5WJHQA#issuecomment-527209408, or mute the thread https://github.com/notifications/unsubscribe-auth/ABIXFVQG5QK36XTB66UQU7LQHVDPNANCNFSM4IS6NN3A .

claremacrae commented 5 years ago

Thanks - don't worry, it definitely wasn't worth spending your time changing the commit messages... And also don't worry if you forget in future - it's easily done, especially if not using that notation on other projects in between.