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

Remove indexed maps that are not used along the code #178

Merged
merged 4 commits into from
Oct 18, 2022

Conversation

jparisu
Copy link
Contributor

@jparisu jparisu commented Oct 14, 2022

Copy link
Member

@MiguelCompany MiguelCompany left a comment

Choose a reason for hiding this comment

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

Two general comments:

  1. If we are removing the collections, shouldn't we just remove the corresponding tests instead of modifying them?
  2. Linters

@jparisu jparisu temporarily deployed to codecov October 18, 2022 10:59 Inactive
MiguelCompany
MiguelCompany previously approved these changes Oct 18, 2022
Copy link
Member

@MiguelCompany MiguelCompany 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 CI. We'll leave the tests there till we check whether we have other tests checking the same things indirectly.

jparisu added 3 commits October 18, 2022 14:08
Signed-off-by: jparisu <javierparis@eprosima.com>
Signed-off-by: jparisu <javierparis@eprosima.com>
Signed-off-by: jparisu <javierparis@eprosima.com>
@jparisu jparisu force-pushed the feature/remove-indexed-maps branch from 82b8375 to 9694f24 Compare October 18, 2022 12:08
@jparisu jparisu temporarily deployed to codecov October 18, 2022 12:08 Inactive
MiguelCompany
MiguelCompany previously approved these changes Oct 18, 2022
Copy link
Member

@MiguelCompany MiguelCompany left a comment

Choose a reason for hiding this comment

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

LGTM

Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>
@MiguelCompany MiguelCompany force-pushed the feature/remove-indexed-maps branch from f267a35 to 736d776 Compare October 18, 2022 14:12
@MiguelCompany MiguelCompany temporarily deployed to codecov October 18, 2022 14:12 Inactive
@codecov
Copy link

codecov bot commented Oct 18, 2022

Codecov Report

Base: 0.00% // Head: 57.82% // Increases project coverage by +57.82% 🎉

Coverage data is based on head (736d776) compared to base (4676eb2).
Patch has no changes to coverable lines.

Additional details and impacted files
@@            Coverage Diff            @@
##           main     #178       +/-   ##
=========================================
+ Coverage      0   57.82%   +57.82%     
=========================================
  Files         0       31       +31     
  Lines         0     4320     +4320     
  Branches      0     2310     +2310     
=========================================
+ Hits          0     2498     +2498     
- Misses        0       60       +60     
- Partials      0     1762     +1762     
Impacted Files Coverage Δ
src/cpp/database/database.cpp 51.47% <ø> (ø)
src/cpp/database/database.hpp 65.06% <ø> (ø)
...c/cpp/subscriber/StatisticsParticipantListener.cpp 48.67% <0.00%> (ø)
src/cpp/database/data.cpp 100.00% <0.00%> (ø)
src/cpp/subscriber/StatisticsReaderListener.cpp 50.00% <0.00%> (ø)
src/cpp/database/database_queue.cpp 54.80% <0.00%> (ø)
...lude/fastdds_statistics_backend/types/EntityId.hpp 100.00% <0.00%> (ø)
.../cpp/detail/data_aggregators/MaximumAggregator.ipp 100.00% <0.00%> (ø)
src/cpp/database/database_queue.hpp 63.69% <0.00%> (ø)
... and 23 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@MiguelCompany MiguelCompany merged commit 1e368df into main Oct 18, 2022
@MiguelCompany MiguelCompany deleted the feature/remove-indexed-maps branch October 18, 2022 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants