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.

Sign up for GitHub

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

Jump to bottom

Replace common::Time with std::chrono #135

Merged
merged 6 commits into from
Sep 17, 2020
Merged

Replace common::Time with std::chrono #135

merged 6 commits into from
Sep 17, 2020

Conversation

ahcorde
Copy link
Contributor

@ahcorde ahcorde commented Sep 2, 2020

This PR is part of this issue gazebosim/gz-common#61. The idea it's to deprecate the class common::Time in favor of std::chrono.

Depends on:

Signed-off-by: ahcorde [email protected]

@ahcorde ahcorde added enhancement New feature or request 🔮 dome Ignition Dome labels Sep 2, 2020
@ahcorde ahcorde requested a review from scpeters September 2, 2020 12:24
@ahcorde ahcorde requested a review from iche033 as a code owner September 2, 2020 12:24
@ahcorde ahcorde self-assigned this Sep 2, 2020
@ahcorde ahcorde mentioned this pull request Sep 2, 2020
Closed
@codecov
Copy link

codecov bot commented Sep 2, 2020 •
edited
Loading

Codecov Report

Merging #135 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #135      +/-   ##
==========================================
+ Coverage   52.09%   52.11%   +0.01%     
==========================================
  Files         143      143              
  Lines       13070    13075       +5     
==========================================
+ Hits         6809     6814       +5     
  Misses       6261     6261
Impacted Files Coverage Δ
include/ignition/rendering/Scene.hh 0.00% <ø> (ø)
include/ignition/rendering/base/BaseScene.hh 0.00% <ø> (ø)
src/base/BaseScene.cc 75.26% <100.00%> (+0.22%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5929c3f...f063b08. Read the comment docs.

@chapulina chapulina added needs upstream release Blocked by a release of an upstream library beta Targeting beta release of upcoming collection labels Sep 5, 2020
@ahcorde
Added feedback
74a7fc7 
Signed-off-by: ahcorde <[email protected]>
chapulina
chapulina reviewed Sep 15, 2020
View reviewed changes
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

I know the deprecated functions don't have tests, but this could be a good opportunity to add simple unit tests for the new functions, if it doesn't take too much time.

include/ignition/rendering/Scene.hh Outdated Show resolved Hide resolved
@ahcorde ahcorde requested a review from chapulina September 16, 2020 09:47
chapulina
chapulina reviewed Sep 16, 2020
View reviewed changes
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

We should add a note to the migration guide.

Also bump the ign-math version on CMakeLists to 6.6.0.

include/ignition/rendering/base/BaseScene.hh Outdated Show resolved Hide resolved
src/base/BaseScene.cc Show resolved Hide resolved
chapulina added a commit to gazebo-tooling/release-tools that referenced this pull request Sep 16, 2020
@chapulina
Testing gazebosim/gz-rendering#135
708b300 
Signed-off-by: Louise Poubel <[email protected]>
@chapulina
Copy link
Contributor

chapulina commented Sep 16, 2020 •
edited
Loading

ign-math 6.6.0~pre1 binaries are out, testing this PR against that here:

@chapulina chapulina mentioned this pull request Sep 17, 2020
Merged
@chapulina chapulina merged commit 9f242c9 into master Sep 17, 2020
@chapulina chapulina deleted the ahcorde/std_chrono branch September 17, 2020 23:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chapulina chapulina chapulina approved these changes

@scpeters scpeters Awaiting requested review from scpeters

@iche033 iche033 Awaiting requested review from iche033

Assignees

@ahcorde ahcorde

Labels
beta Targeting beta release of upcoming collection 🔮 dome Ignition Dome enhancement New feature or request needs upstream release Blocked by a release of an upstream library
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ahcorde @chapulina @iche033