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

Quality declaration #47

Open
wants to merge 1 commit into
base: master
from
Open

Quality declaration #47

wants to merge 1 commit into from

Conversation

@brawner
Copy link

brawner commented Feb 29, 2020

This includes the quality declaration description to help bring this package to quality level 1. For the most part, this package is not missing large pieces for quality level 1. Code coverage is not great, and it's also not yet clear what performance tests should be included in this package. I couldn't find any obvious API issues.

Depends on #46

@brawner brawner requested a review from ros2/aws-robotics-code-owners as a code owner Feb 29, 2020
@brawner brawner force-pushed the brawner:quality_declaration branch from 3cca399 to 46089b4 Feb 29, 2020
Copy link
Collaborator

emersonknapp left a comment

This looks good to me on passing CI

@wjwwood

This comment has been minimized.

Copy link
Member

wjwwood commented Mar 2, 2020

Is the plan to merge this as-is even though it is "aspirational"? Currently things like:

rcpputils uses semver according to the recommendation for ROS Core packages in the ROS 2 Developer Guide, and is at or above a stable version, i.e. >= 1.0.0.

Are not true, as in this case rcpputils is not yet >= 1.0.0.

I'm not sure what we should do, but it seems misleading to merge this so that it's on the master branch when all the cases aren't actually done yet.

On the other repositories my plan was to either leave the pr open until the items could get addressed, or change the quality declarations to be accurate for the current state before merging, even if that means we can't declare it level 1 for now.

@emersonknapp

This comment has been minimized.

Copy link
Collaborator

emersonknapp commented Mar 2, 2020

Leaving it open or modifying to be accurate are both acceptable options to me. Maybe a slight preference towards "getting something in" - so, accurate info, with aspirations potentially called out.

@brawner

This comment has been minimized.

Copy link
Author

brawner commented Mar 3, 2020

I'll leave it up to others. I'm happy either leaving this open or editing this PR to just include the pieces that are true.

@brawner brawner force-pushed the brawner:quality_declaration branch from 46089b4 to b04e564 Mar 3, 2020
@brawner

This comment has been minimized.

Copy link
Author

brawner commented Mar 3, 2020

Rebasing this PR

Signed-off-by: Stephen Brawner <brawner@gmail.com>
@brawner brawner force-pushed the brawner:quality_declaration branch from b04e564 to 3fd32fd Mar 7, 2020
@brawner

This comment has been minimized.

Copy link
Author

brawner commented Mar 7, 2020

Rebasing on master

Copy link
Contributor

thomas-moulard left a comment

LGTM, a few nits


### Public API Documentation

TODO upload docs

This comment has been minimized.

Copy link
@thomas-moulard

thomas-moulard Mar 9, 2020

Contributor

remove for now and open an issue to track?


### Performance

TODO document performance tests

This comment has been minimized.

Copy link
@thomas-moulard

thomas-moulard Mar 9, 2020

Contributor

ditto


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 on [codecov.io](https://codecov.io/gh/j-rivero/rcpputils):

This comment has been minimized.

Copy link
@thomas-moulard

thomas-moulard Mar 9, 2020

Contributor

Why is it on Jose's account, seems fragile. Could we have that from the action in this repo?

@Blast545

This comment has been minimized.

Copy link

Blast545 commented Mar 9, 2020

I'm not sure what we should do, but it seems misleading to merge this so that it's on the master branch when all the cases aren't actually done yet.

I agree with William on this, I personally think that this should remain open as "Aspirational Quality Declaration" (as with other packages). If needed, a new PR could be opened with the current category, probably Level 3

@ros-discourse

This comment has been minimized.

Copy link

ros-discourse commented Mar 9, 2020

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/rfc-rep-2004-package-quality-categories/13150/1

@thomas-moulard

This comment has been minimized.

Copy link
Contributor

thomas-moulard commented Mar 9, 2020

I'm not sure what we should do, but it seems misleading to merge this so that it's on the master branch when all the cases aren't actually done yet.

I agree with William on this, I personally think that this should remain open as "Aspirational Quality Declaration" (as with other packages). If needed, a new PR could be opened with the current category, probably Level 3

+1, let's not merge thing which aren't true. One goal of this PR is to actually decide which quality level we're currently at.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.