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

Address sdformat11 deprecations #321

Merged
merged 2 commits into from
Apr 28, 2021
Merged

Conversation

diegoferigo
Copy link
Member

@diegoferigo diegoferigo commented Apr 1, 2021

Updates the code to sdformat11, and particularly complies with gazebosim/sdformat#444.


This PR depends on #307 for Edifice support, and #318 for disabling Dome in CI.

Copy link
Member

@traversaro traversaro left a comment

Choose a reason for hiding this comment

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

If I understand correctly, this changes the public ABI and API of the C++ Scenario package. If the project follows semantic versioning (see #81 (comment)) this would mean that the major version needs to bump? If instead the compatibility policy is not semantic versioning (even for a subset of the project) it would be convenient to document it in the README, for downstream packages.

@diegoferigo
Copy link
Member Author

diegoferigo commented Apr 28, 2021

Uhm, the helpers.h is not a public header, but utils.h is public. So, yes, there's actually an API change:

    // OLD
    std::string getModelNameFromSdf(const std::string& fileName,
                                    const size_t modelIndex = 0);
    // NEW
    std::string getModelNameFromSdf(const std::string& fileName);

I'm debating about how to handle the process. The PR targets devel (based on Ignition Edifice which requires sdformat11), and sdformat11 deprecated the functionality updated in this PR.

I think it's safe saying that we follow semantic versioning only for the ScenarIO interface. This is not part of it and I agree that we should document it somewhere.

@traversaro
Copy link
Member

traversaro commented Apr 28, 2021

I think it's safe saying that we follow semantic versioning only for the ScenarIO interface. This is not part of it and I agree that we should document it somewhere.

For convenient of downstream packages, it would be convenient to specify a API/ABI policy also for the part of ScenarIO that are not part of the interface, even just something as limited as API/ABI is not broken as part of the same patch release x.y.z (that is kind of implicit if you use x.y.z version numbers).

@diegoferigo
Copy link
Member Author

Yep, while you were commenting I opened #337 as reminder.

@diegoferigo diegoferigo merged commit f3e72b0 into devel Apr 28, 2021
@diegoferigo diegoferigo deleted the fix/edifice_deprecations branch April 28, 2021 07:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bump to Edifice: deprecation warnings
2 participants