-
Notifications
You must be signed in to change notification settings - Fork 59
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
Replaced common::Time for std::chrono #41
Conversation
Signed-off-by: ahcorde <[email protected]>
Signed-off-by: ahcorde <[email protected]>
hmm I think we should follow the tick-tock release cycle and deprecate the functions in Dome first then remove them in ignition E |
it could be nice to add a helper function to ign-msgs that populates msgs::Time from |
We already have some in ignition/math/Helpers.hh, so I'd suggest we add more there too. |
Signed-off-by: ahcorde <[email protected]>
Signed-off-by: ahcorde <[email protected]>
Signed-off-by: ahcorde <[email protected]>
Signed-off-by: ahcorde <[email protected]>
…ics/ign-sensors into ahcorde/std_chrono
Codecov Report
@@ Coverage Diff @@
## master #41 +/- ##
==========================================
- Coverage 77.67% 76.32% -1.35%
==========================================
Files 23 23
Lines 2302 2319 +17
==========================================
- Hits 1788 1770 -18
- Misses 514 549 +35
Continue to review full report at Codecov.
|
I can see some Ogre errors:
@iche033 any thoughts? |
I started seeing those errors happening recently on our r2d2 jenkins machine. For other PRs, I just take it offline and rerun the build so the tests are run on a different machine. cc current build farmer @caguero |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The public functions need to be deprecated. Be sure to add a note to the migration guide too. We don't usually test the deprecated functions so I think the tests can remain as they are.
Signed-off-by: ahcorde <[email protected]>
…ics/ign-sensors into ahcorde/std_chrono
Signed-off-by: ahcorde <[email protected]>
Signed-off-by: ahcorde <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still missing some deprecations, but I think we're very close
Signed-off-by: ahcorde <[email protected]>
Signed-off-by: ahcorde <[email protected]>
Signed-off-by: ahcorde <[email protected]>
Signed-off-by: ahcorde <[email protected]>
Signed-off-by: ahcorde <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just have the 2 minor comments below that I'll commit now. Then wait for CI and merge.
CC @iche033
Signed-off-by: Louise Poubel <[email protected]>
This PR is part of this issue gazebosim/gz-common#61. The idea it's to deprecate the class
common::Time
in favor ofstd::chrono
.Related with gazebosim/gz-common#90
Signed-off-by: ahcorde [email protected]