Skip to content

AxisAlignedBox: deprecate unimplemented methods#261

Merged
scpeters merged 7 commits intoign-math6from
ahcorde/clean/axisAlignedbox
Oct 15, 2021
Merged

AxisAlignedBox: deprecate unimplemented methods#261
scpeters merged 7 commits intoign-math6from
ahcorde/clean/axisAlignedbox

Conversation

@ahcorde
Copy link
Copy Markdown
Contributor

@ahcorde ahcorde commented Oct 11, 2021

Signed-off-by: ahcorde ahcorde@gmail.com

🦟 Bug fix

Fixes #

Summary

Clean some unimplemented methods in AxisAlignedBox

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge

@ahcorde
Clean some unimplemented methods
b6274e9 
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde ahcorde requested review from azeey and scpeters October 11, 2021 13:32
@ahcorde ahcorde self-assigned this Oct 11, 2021
@github-actions github-actions bot added Gazebo 1️1️ Dependency of Gazebo classic version 11 🌱 garden Ignition Garden 🏢 edifice Ignition Edifice 🏯 fortress Ignition Fortress 🏰 citadel Ignition Citadel 🔮 dome Ignition Dome labels Oct 11, 2021
@codecov
Copy link
Copy Markdown

codecov bot commented Oct 11, 2021 •
edited
Loading

Codecov Report

Merging #261 (67446f5) into ign-math6 (23bf73a) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           ign-math6     #261   +/-   ##
==========================================
  Coverage      99.41%   99.41%           
==========================================
  Files             67       67           
  Lines           6374     6374           
==========================================
  Hits            6337     6337           
  Misses            37       37
Impacted Files Coverage Δ
include/ignition/math/AxisAlignedBox.hh 100.00% <ø> (ø)

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 23bf73a...67446f5. Read the comment docs.

chapulina
chapulina previously requested changes Oct 11, 2021
View reviewed changes
Copy link
Copy Markdown
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.

This time the ABI checker complained, probably because the class is not header-only:

https://build.osrfoundation.org/job/ignition_math-abichecker-any_to_any-ubuntu_auto-amd64/616/API_5fABI_20report/

@ahcorde
Copy link
Copy Markdown
Contributor Author

ahcorde commented Oct 14, 2021

what do you think @scpeters about this PR ?

@scpeters
Copy link
Copy Markdown
Member

scpeters commented Oct 14, 2021

I feel like we should be able to do this, so I'm not sure why the ABI checker complains

cc @j-rivero any ideas?

@j-rivero
Copy link
Copy Markdown
Contributor

j-rivero commented Oct 14, 2021

I feel like we should be able to do this, so I'm not sure why the ABI checker complains

cc @j-rivero any ideas?

If I understand this correctly, we are removing function declarations from a public header installed in the user system which don't have a definition, is this right? This would explain why the abi-checker is happy with respect to ABI but raise warnings in the "Source compatibility" (API).

Removed Symbols  5 
AxisAlignedBox.hh
namespace ignition::math::v6
AxisAlignedBox::DensityFromMass ( double const _mass ) const
_ZNK8ignition4math2v614AxisAlignedBox15DensityFromMassEd

AxisAlignedBox::MassMatrix ( MassMatrix3d& _massMat ) const
_ZNK8ignition4math2v614AxisAlignedBox10MassMatrixERNS1_11MassMatrix3IdEE

AxisAlignedBox::Material ( ) const
_ZNK8ignition4math2v614AxisAlignedBox8MaterialEv

AxisAlignedBox::SetDensityFromMass ( double const _mass )
_ZN8ignition4math2v614AxisAlignedBox18SetDensityFromMassEd

AxisAlignedBox::SetMaterial ( Material const& _mat )
_ZN8ignition4math2v614AxisAlignedBox11SetMaterialERKNS1_8MaterialE

It's quite a corner case and it's not easy to find a decent use-case to go against the removal but someone could be using these declarations and implementing these methods in their own code ... does not sound very convenient but it's possible and in the case, we are breaking the API for them.

Unless we really need this, my proposal is to move to the next major version.

@scpeters
Copy link
Copy Markdown
Member

scpeters commented Oct 14, 2021

I think it would be fair to mark these APIs as deprecated

@ahcorde ahcorde changed the title Clean some unimplemented methods in AxisAlignedBox marked as deprecated unimplemented methods in AxisAlignedBox Oct 15, 2021
@ahcorde ahcorde requested a review from chapulina October 15, 2021 10:24
@ahcorde
Copy link
Copy Markdown
Contributor Author

ahcorde commented Oct 15, 2021

should I target main?

j-rivero
j-rivero reviewed Oct 15, 2021
View reviewed changes
@ahcorde
doxygen
ed3f0d1 
Signed-off-by: ahcorde <ahcorde@gmail.com>
j-rivero
j-rivero approved these changes Oct 15, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@j-rivero j-rivero left a comment

Choose a reason for hiding this comment

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

I think is fine target this to mah6 and release the deprecation in a MINOR due to the weird nature of the unimplemented methods. Up to maintainers but I don't see to much drama.

@scpeters
Copy link
Copy Markdown
Member

scpeters commented Oct 15, 2021

I think is fine target this to mah6 and release the deprecation in a MINOR due to the weird nature of the unimplemented methods. Up to maintainers but I don't see to much drama.

agreed, we can deprecate these in ign-math6 because they are weird

@scpeters scpeters dismissed chapulina’s stale review October 15, 2021 16:58

ABI job is fixed

@scpeters scpeters changed the title marked as deprecated unimplemented methods in AxisAlignedBox AxisAlignedBox: deprecate unimplemented methods Oct 15, 2021
@scpeters scpeters merged commit a6b0cff into ign-math6 Oct 15, 2021
@scpeters scpeters deleted the ahcorde/clean/axisAlignedbox branch October 15, 2021 18:32
@ahcorde ahcorde mentioned this pull request Oct 29, 2021
Closed
45 tasks
@osrf-triage
Copy link
Copy Markdown

osrf-triage commented Mar 1, 2022

This pull request has been mentioned on Gazebo Community. There might be relevant details there:

https://community.gazebosim.org/t/new-ignition-releases-2022-03-01-citadel-edifice-fortress/1313/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@j-rivero j-rivero j-rivero approved these changes

@scpeters scpeters scpeters approved these changes

@azeey azeey Awaiting requested review from azeey

@chapulina chapulina Awaiting requested review from chapulina

Assignees

@ahcorde ahcorde

Labels

🏰 citadel Ignition Citadel 🔮 dome Ignition Dome 🏢 edifice Ignition Edifice 🏯 fortress Ignition Fortress 🌱 garden Ignition Garden Gazebo 1️1️ Dependency of Gazebo classic version 11

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@ahcorde @scpeters @j-rivero @osrf-triage @chapulina