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

Refactored timeout.h: added template ctr and removed redundant ctrs (#1129) #1131

Merged
merged 1 commit into from
Oct 27, 2024

Conversation

laxerhd
Copy link
Contributor

@laxerhd laxerhd commented Oct 26, 2024

I have refactored the constructors of the Timeout class:

  • Added template constructor that accepts any std::chrono::duration type and converts it to std::chrono::milliseconds
  • Removed two redundant constructors for milliseconds and seconds
  • Ctr for int32_t was untouched

I am not sure if we still want the NOLINTNEXTLINE(...) above the template ctr, but for consistency I have added it back.

  • I have made sure to run all test cases like described in the readme.

Fixes #1129.

@laxerhd laxerhd changed the title ´Refactored´ timeout.h: added template ctr and removed redundant ctrs (#1129) Refactored timeout.h: added template ctr and removed redundant ctrs (#1129) Oct 26, 2024
README.md Outdated
@@ -7,7 +7,7 @@
## Announcements

* This project is being maintained by [Fabian Sauter](https://github.com/com8) and [Kilian Traub](https://github.com/KingKili).
* For quick help, and discussion libcpr also offer a [gitter](https://gitter.im/libcpr/community?utm_source=share-link&utm_medium=link&utm_campaign=share-link) chat.
* For quick help, and discussion libcpr also offers a [gitter](https://gitter.im/libcpr/community?utm_source=share-link&utm_medium=link&utm_campaign=share-link) chat.
Copy link
Member

Choose a reason for hiding this comment

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

This stuff should not be included. As the other PR is now merged, try rebasing on the latest master or drop the commit if possible.

Copy link
Contributor Author

@laxerhd laxerhd Oct 27, 2024

Choose a reason for hiding this comment

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

Sorry, was a mistake on my side, I didn't want to include it here...

@laxerhd
Copy link
Contributor Author

laxerhd commented Oct 27, 2024

The latest commit should now be dropped.

Copy link
Member

@COM8 COM8 left a comment

Choose a reason for hiding this comment

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

Approved. Thanks.

To answer your question:
The NOLINT is still required since else clang-tidy will complain we should add explicit in front of the ctr to prevent an C++ offering an implicit cast from a Chrono duration to a timeout.

But in this case we want an implicit conversation to be possible.

@COM8 COM8 merged commit 372b54c into libcpr:master Oct 27, 2024
103 of 104 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Why does cpr::Timeout has two chrono bases ctors?
2 participants