Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[21053] Move DR TypeConsistencyEnforcement & DataRepresentation from TypeConsistency to DataReaderQos #4823

Merged
merged 2 commits into from
May 27, 2024

Conversation

JesusPoderoso
Copy link
Contributor

@JesusPoderoso JesusPoderoso commented May 21, 2024

Description

This PR updates the DataReaderQoS reformatting the TypeConsistencyQos:

  1. DataReaderQos shall have:
    • TypeConsistencyEnforcementQosPolicy::type_consistency
    • DataRepresentationQosPolicy::representation
  2. DataReader::TypeConsistencyQos shall be removed

Contributor Checklist

  • Commit messages follow the project guidelines.
  • The code follows the style guidelines of this project.
  • Tests that thoroughly check the new feature have been added/Regression tests checking the bug and its fix have been added; the added tests pass locally
  • Any new/modified methods have been properly documented using Doxygen.
  • N/A Any new configuration API has an equivalent XML API (with the corresponding XSD extension)
  • ❌ Changes are backport compatible: they do NOT break ABI nor change library core behavior.
  • ❌ Changes are API compatible.
  • New feature has been added to the versions.md file (if applicable).
  • New feature has been documented/Current behavior is correctly described in the documentation.
  • N/A Applicable backports have been included in the description.

Reviewer Checklist

  • The PR has a milestone assigned.
  • The title and description correctly express the PR's purpose.
  • Check contributor checklist is correct.
  • Check CI results: changes do not issue any warning.
  • Check CI results: failing tests are unrelated with the changes.

…entationQosPolicy to correct place

Signed-off-by: JesusPoderoso <jesuspoderoso@eprosima.com>
@JesusPoderoso JesusPoderoso added this to the v3.0.0 milestone May 21, 2024
@JesusPoderoso JesusPoderoso self-assigned this May 21, 2024
@JesusPoderoso JesusPoderoso requested a review from Mario-DL May 21, 2024 14:36
@github-actions github-actions bot added the ci-pending PR which CI is running label May 21, 2024
Copy link
Member

@Mario-DL Mario-DL left a comment

Choose a reason for hiding this comment

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

Good job in this PR.
Leaving some corrections and a suggestion. In addition, some considerations:

  • I think that the changes in this PR should be reflected in the versions.md
  • As this PR is targeting 3.0.0, we can break API, which should be left unmarked in the checklist as this PR is breaking.
  • Before merging this PR it would be nice if we could, at least in local, compile other products. For instance, we should also link a PR to Shapes Demo overcoming the API break.
  • If possible, edit PR's title since ... seems to be the commit message directly.

@JesusPoderoso JesusPoderoso changed the title [21053] Move DR TypeConsistencyEnforcementQosPolicy & DataRepresentationQoSPolicy… [21053] Move DR TypeConsistencyEnforcement & DataRepresentation from TypeConsistency to DataReaderQos May 23, 2024
Signed-off-by: JesusPoderoso <jesuspoderoso@eprosima.com>
@JesusPoderoso
Copy link
Contributor Author

As this PR is targeting 3.0.0, we can break API, which should be left unmarked in the checklist as this PR is breaking.

I think it is correctly checked, see following comment section of the PR template:

<!--
- If any of the elements of the following checklist is not applicable, substitute the checkbox [ ] by _N/A_:
- If any of the elements of the following checklist is not fulfilled on purpose, please provide a reason
  and substitute the checkbox [ ] with ❌: or __NO__:.
-->

@Mario-DL
Copy link
Member

@richiprosima please test_3 this

Copy link
Member

@Mario-DL Mario-DL 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 green Jenkins CI

@Mario-DL
Copy link
Member

@richiprosima please test_3 windows

@JesusPoderoso JesusPoderoso added ready-to-merge Ready to be merged. CI and changes have been reviewed and approved. and removed ci-pending PR which CI is running labels May 24, 2024
@EduPonz EduPonz merged commit 623685d into master May 27, 2024
11 of 15 checks passed
@EduPonz EduPonz deleted the feature/21053 branch May 27, 2024 05:36
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
ready-to-merge Ready to be merged. CI and changes have been reviewed and approved.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants