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

Deprecated common:Time #90

Merged
merged 17 commits into from
Sep 18, 2020
Merged

Deprecated common:Time #90

merged 17 commits into from
Sep 18, 2020

Conversation

ahcorde
Copy link
Contributor

@ahcorde ahcorde commented Aug 25, 2020

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

This two functions convert from time_point to seconds and nanoseconds and viceversa.

Signed-off-by: ahcorde [email protected]

@ahcorde ahcorde added enhancement New feature or request 🔮 dome Ignition Dome labels Aug 25, 2020
@ahcorde ahcorde requested a review from mjcarroll as a code owner August 25, 2020 18:09
@ahcorde ahcorde self-assigned this Aug 25, 2020
@github-actions github-actions bot added 🏰 citadel Ignition Citadel 📜 blueprint Ignition Blueprint labels Aug 25, 2020
This was referenced Aug 25, 2020
Merged
Merged
@codecov
Copy link

codecov bot commented Aug 25, 2020 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (master@512f077). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master      #90   +/-   ##
=========================================
  Coverage          ?   73.78%           
=========================================
  Files             ?       68           
  Lines             ?     9209           
  Branches          ?        0           
=========================================
  Hits              ?     6795           
  Misses            ?     2414           
  Partials          ?        0

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 512f077...6805de0. Read the comment docs.

@ahcorde ahcorde mentioned this pull request Aug 25, 2020
Closed
chapulina
chapulina suggested changes Aug 25, 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 like the ideas on this PR, but I suggest we move the new functions to ign-math.

include/ignition/common/Time.hh Outdated Show resolved Hide resolved
include/ignition/common/Time.hh Outdated Show resolved Hide resolved
@ahcorde ahcorde force-pushed the ahcorde/std_chrono branch from cf46105 to 3d67a25 Compare August 26, 2020 07:17
@ahcorde ahcorde requested a review from maryaB-osr as a code owner August 26, 2020 07:17
@ahcorde ahcorde changed the base branch from ign-common3 to master August 26, 2020 07:17
@ahcorde ahcorde force-pushed the ahcorde/std_chrono branch from 3d67a25 to a370f08 Compare August 26, 2020 07:18
@ahcorde ahcorde changed the title Added static function to handle std::chrono::time_point Deprecated common:Time Aug 26, 2020
@ahcorde
Copy link
Contributor Author

ahcorde commented Aug 26, 2020

  • Remove two new methods (moved to ign-math).
  • Deprecated constructor and static functions.
  • Retarget branch to master

This was referenced Aug 26, 2020
Merged
Merged
@chapulina chapulina removed 🏰 citadel Ignition Citadel 📜 blueprint Ignition Blueprint 🔮 dome Ignition Dome labels Aug 31, 2020
chapulina
chapulina suggested changes Aug 31, 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 resolve the introduced deprecation warnings before merging:

https://build.osrfoundation.org/job/ignition_common-ci-pr_any-ubuntu_auto-amd64/370/gcc/

Also, I think it would be a good idea to hold on merging this until we are sure that we can remove common::Time from all existing API on other libraries.

@ahcorde
Copy link
Contributor Author

ahcorde commented Sep 2, 2020

@chapulina, This issue is the meta-ticket to follow all the libraries that currently are using common::Time and it points to the related PRs to replace the use of common::Time with std::chrono

@ahcorde ahcorde requested a review from chapulina September 2, 2020 13:40
@ahcorde
fixed action
f253e4d 
Signed-off-by: ahcorde <[email protected]>
@ahcorde
fixed action
c798f50 
Signed-off-by: ahcorde <[email protected]>
@ahcorde
Copy link
Contributor Author

ahcorde commented Sep 2, 2020

I fixed the after_make.sh script in this PR, but I think the right solution is in this other PR gazebo-tooling/action-gz-ci#22

chapulina
chapulina suggested changes Sep 8, 2020
View reviewed changes
src/WorkerPool_TEST.cc Outdated Show resolved Hide resolved
src/Timer.cc Outdated Show resolved Hide resolved
src/Timer.cc Outdated Show resolved Hide resolved
@ahcorde ahcorde requested a review from chapulina September 16, 2020 07:41
chapulina
chapulina reviewed Sep 17, 2020
View reviewed changes
include/ignition/common/Timer.hh Outdated Show resolved Hide resolved
src/WorkerPool.cc Show resolved Hide resolved
@ahcorde ahcorde requested a review from chapulina September 17, 2020 08:07
@ahcorde
Copy link
Contributor Author

ahcorde commented Sep 17, 2020

@osrf-jenkins run tests

@chapulina
bump ign-math version
58a9096 
Signed-off-by: Louise Poubel <[email protected]>
chapulina
chapulina approved these changes Sep 18, 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.

LGTM with happy CI. I just have one minor comment about code duplication.

src/Timer.cc Show resolved Hide resolved
@ahcorde
Copy link
Contributor Author

ahcorde commented Sep 18, 2020

@osrf-jenkins run tests

@chapulina chapulina merged commit 526c2ac into master Sep 18, 2020
@chapulina chapulina deleted the ahcorde/std_chrono branch September 18, 2020 16:50
@chapulina chapulina mentioned this pull request Jul 20, 2022
Merged
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iche033 iche033 iche033 left review comments

@chapulina chapulina chapulina approved these changes

@mjcarroll mjcarroll mjcarroll approved these changes

@maryaB-osr maryaB-osr Awaiting requested review from maryaB-osr

Assignees

@ahcorde ahcorde

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

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