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

Fast-CDR external dependency Quality Declaration #360

Closed
wants to merge 6 commits into from

Conversation

Blast545
Copy link
Contributor

@Blast545 Blast545 commented Apr 1, 2020

This is a quality declaration for eProsima Fast-CDR external dependency for ROS2 packages as described in the proposed REP-2004.

This declaration represents some thoughts regarding the current state of this external dependency and how this affect Quality Level 1 for ROS2 packages, feedback is well received.

Signed-off-by: Jorge Perez jjperez@ekumenlabs.com

Signed-off-by: Jorge Perez <jjperez@ekumenlabs.com>
Copy link
Contributor

@brawner brawner left a comment

Choose a reason for hiding this comment

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

It looks like a lot of my comments from the FastRTPS PR can also be applied here. Could you update this with the most current style and carry over any relevant changes from the other PR and then I'll take a deeper dive?

Quality_declaration_fastcdr.md Outdated Show resolved Hide resolved
Signed-off-by: Jorge Perez <jjperez@ekumenlabs.com>
Signed-off-by: Jorge Perez <jjperez@ekumenlabs.com>
@Blast545 Blast545 requested review from brawner and ahcorde May 1, 2020 14:32
Copy link
Contributor

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

LGTM, just a small change

Quality_declaration_fastcdr.md Outdated Show resolved Hide resolved

As stated in their public repository, *eProsima FastCDR is a C++ library that provides two serialization mechanisms. One is the standard CDR serialization mechanism, while the other is a faster implementation that modifies the standard*.

## Summary
Copy link
Contributor

Choose a reason for hiding this comment

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

For large paragraphs like these below, it might be better to have once sentence per-line so that the diff is easier to read.

Quality_declaration_fastcdr.md Outdated Show resolved Hide resolved
Quality_declaration_fastcdr.md Show resolved Hide resolved
Quality_declaration_fastcdr.md Outdated Show resolved Hide resolved
Quality_declaration_fastcdr.md Outdated Show resolved Hide resolved
Quality_declaration_fastcdr.md Outdated Show resolved Hide resolved
Quality_declaration_fastcdr.md Outdated Show resolved Hide resolved
Quality_declaration_fastcdr.md Outdated Show resolved Hide resolved
Signed-off-by: Jorge Perez <jjperez@ekumenlabs.com>
@Blast545 Blast545 requested a review from brawner May 1, 2020 21:35
Copy link
Contributor

@brawner brawner left a comment

Choose a reason for hiding this comment

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

Minor nits, but otherwise looks good to me.

Quality_declaration_fastcdr.md Outdated Show resolved Hide resolved
Quality_declaration_fastcdr.md Outdated Show resolved Hide resolved
Quality_declaration_fastcdr.md Outdated Show resolved Hide resolved
Quality_declaration_fastcdr.md Outdated Show resolved Hide resolved
@ahcorde
Copy link
Contributor

ahcorde commented May 4, 2020

add the QD to the README.md

Blast545 added 2 commits May 4, 2020 18:51
Signed-off-by: Jorge Perez <jjperez@ekumenlabs.com>
Signed-off-by: Jorge Perez <jjperez@ekumenlabs.com>
@EduPonz
Copy link

EduPonz commented Jan 19, 2021

Due to Fast DDS reaching QL1 (eProsima/Fast-DDS#1610), I think this PR can be closed

@Blast545 Blast545 closed this Jan 21, 2021
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.

4 participants