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.

Sign up for GitHub

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

Jump to bottom

Update Quality Declaration to reflect QL 2 (#160). #160

Merged
merged 3 commits into from
Jun 26, 2020
Merged

Update Quality Declaration to reflect QL 2 (#160). #160

merged 3 commits into from
Jun 26, 2020

Conversation

hidmic
Copy link
Contributor

@hidmic hidmic commented Jun 5, 2020

Precisely what the title says.

Blast545
Blast545 reviewed Jun 5, 2020
View reviewed changes
QUALITY_DECLARATION.md Outdated
@@ -2,7 +2,7 @@ This document is a declaration of software quality for the `class_loader` packag

# class_loader Quality Declaration

The package `class_loader` claims to be in the **Quality Level 4** category.
The package `class_loader` claims to be in the **Quality Level 2** category.

Below are the rationales, notes, and caveats for this claim, organized by each requirement listed in the [Package Quality Categories in REP-2004](https://www.ros.org/reps/rep-2004.html) of the ROS2 developer guide.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Below are the rationales, notes, and caveats for this claim, organized by each requirement listed in the [Package Quality Categories in REP-2004](https://www.ros.org/reps/rep-2004.html) of the ROS2 developer guide.
Below are the rationales, notes, and caveats for this claim, organized by each requirement listed in the [Package Quality Categories in REP-2004](https://index.ros.org/doc/ros2/Contributing/Developer-Guide/#package-quality-categories) of the ROS2 developer guide.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made a tweak. See 873892b.

@Blast545
Copy link
Contributor

Blast545 commented Jun 5, 2020

I'd add a couple of comments I found while reviewing the file, please address them, github interface didn't alllow me to comment directly in the file:

Update 2.i to: All changes will occur through a pull request, check the ROS 2 Developer Guide for additional information.

Update text in 3.i to link to features described in README.md file.

Update 3.ii to say "There is documentation for all of the public API using docblocks"

@Blast545
Copy link
Contributor

Blast545 commented Jun 5, 2020

And this should be merged after rcpputils is updated to Q2 as well.

@chapulina
Copy link

And this should be merged after rcpputils is updated to Q2 as well.

Good call, I missed that when looking at the dependency graph

chapulina
chapulina reviewed Jun 8, 2020
View reviewed changes
QUALITY_DECLARATION.md Outdated
@@ -114,7 +114,7 @@ This includes:

Changes are required to make a best effort to keep or increase coverage before being accepted, but decreases are allowed if properly justified and accepted by reviewers.

Current coverage statistics can be viewed [here](https://ci.ros2.org/job/ci_linux_coverage/85/cobertura/src_ros_class_loader_include_class_loader/) and [here](https://ci.ros2.org/job/ci_linux_coverage/85/cobertura/src_ros_class_loader_include_class_loader/). This package does not yet meet the 95% coverage guideline, but it is currently above 90%.
Current coverage statistics can be viewed [here](https://ci.ros2.org/job/ci_linux_coverage/lastSuccessfulBuild/cobertura/src_ros_class_loader_include_class_loader/). This package does not yet meet the 95% coverage guideline, but it is currently above 90%.
Copy link

Choose a reason for hiding this comment

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

Could you add a link to the developer guide explaining how to calculate coverage? Such as

A description of how coverage statistics are summarized from this page, can be found in the ["ROS 2 Onboarding Guide"](https://index.ros.org/doc/ros2/Contributing/ROS-2-On-boarding-Guide/#note-on-coverage-runs).

See this example: https://github.com/ros2/rcutils/pull/260/files#diff-92440392597dfc3b7adb67bc4ed481b3R118

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 7f6b725.

@hidmic
Copy link
Contributor Author

hidmic commented Jun 11, 2020 •
edited
Loading

Alright, may I ask anyone of the maintainers to merge this PR?

hidmic added 3 commits June 16, 2020 11:17
@hidmic
Update Quality Declaration to reflect QL 2
aa6f47b 
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic
Address peer review comments.
b9db68b 
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic
Add reference to coverage computation documentation.
14d2139 
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic hidmic force-pushed the hidmic/quality-declaration-ql-2 branch from 7f6b725 to 14d2139 Compare June 16, 2020 14:17
@hidmic
Copy link
Contributor Author

hidmic commented Jun 25, 2020

@nuclearsandwich ping.

nuclearsandwich
nuclearsandwich approved these changes Jun 26, 2020
View reviewed changes
Copy link
Member

@nuclearsandwich nuclearsandwich left a comment

Choose a reason for hiding this comment

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

I reviewed only that all of the changes in this QD appear factual. Not that the result correctly updates the quality level from four to two. I trust that the other reviewers who've been doing more on the quality front have done so.

@nuclearsandwich
Copy link
Member

Thanks to the other reviewers. Together with my own review this is ready for merge.

@nuclearsandwich nuclearsandwich changed the title Update Quality Declaration to reflect QL 2 Update Quality Declaration to reflect QL 2 (#160). Jun 26, 2020
@nuclearsandwich nuclearsandwich merged commit dfab32e into ros:ros2 Jun 26, 2020
ahcorde pushed a commit that referenced this pull request Oct 27, 2020
@hidmic @ahcorde
Update Quality Declaration to reflect QL 2 (#160).
76d016b
ahcorde pushed a commit that referenced this pull request Oct 29, 2020
@hidmic @ahcorde
Update Quality Declaration to reflect QL 2 (#160).
ba3d6ba
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Blast545 Blast545 Blast545 approved these changes

@chapulina chapulina chapulina approved these changes

@ahcorde ahcorde ahcorde approved these changes

@nuclearsandwich nuclearsandwich nuclearsandwich approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@hidmic @Blast545 @chapulina @nuclearsandwich @ahcorde