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

Quality declaration #47

Merged
merged 6 commits into from
Apr 27, 2020
Merged

Quality declaration #47

merged 6 commits into from
Apr 27, 2020

Conversation

brawner
Copy link
Contributor

@brawner 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 a team as a code owner February 29, 2020 22:53
emersonknapp
emersonknapp approved these changes Mar 2, 2020
View reviewed changes
Copy link
Contributor

@emersonknapp emersonknapp left a comment

Choose a reason for hiding this comment

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

This looks good to me on passing CI

@wjwwood
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
Copy link
Contributor

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
Copy link
Contributor 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
Copy link
Contributor Author

brawner commented Mar 3, 2020

Rebasing this PR

@brawner
Copy link
Contributor Author

brawner commented Mar 7, 2020

Rebasing on master

thomas-moulard
thomas-moulard approved these changes Mar 9, 2020
View reviewed changes
Copy link

@thomas-moulard thomas-moulard left a comment

Choose a reason for hiding this comment

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

LGTM, a few nits

QUALITY_DECLARATION.md Outdated

### Public API Documentation

TODO upload docs
Copy link

Choose a reason for hiding this comment

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

remove for now and open an issue to track?

QUALITY_DECLARATION.md Outdated

### Performance

TODO document performance tests
Copy link

Choose a reason for hiding this comment

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

ditto

QUALITY_DECLARATION.md Outdated

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):
Copy link

Choose a reason for hiding this comment

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

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

@Blast545
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
Copy link

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
Copy link

thomas-moulard commented Mar 9, 2020 •
edited
Loading

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.

@brawner brawner mentioned this pull request Mar 25, 2020
Closed
19 tasks
@chapulina chapulina mentioned this pull request Apr 2, 2020
Merged
ahcorde
ahcorde reviewed Apr 17, 2020
View reviewed changes
QUALITY_DECLARATION.md Outdated Show resolved Hide resolved
@brawner
First draft quality declaration
e7a0f8d 
Signed-off-by: Stephen Brawner <brawner@gmail.com>
@brawner
QD now describes current status
2851821 
Signed-off-by: Stephen Brawner <brawner@gmail.com>
@brawner
Copy link
Contributor Author

brawner commented Apr 22, 2020

We've decided to transition these aspirational QDs to more current status QDs. For the most part a lot of the content stays the same, but this won't have to sit around as an open PR anymore at least.

@ahcorde
Copy link
Contributor

ahcorde commented Apr 22, 2020 •
edited
Loading

Same comment ros2/rosidl_typesupport#67 (comment)

@brawner
Fixup
bfc95f3 
Signed-off-by: Stephen Brawner <brawner@gmail.com>
@brawner
Fix link
f667481 
Signed-off-by: Stephen Brawner <brawner@gmail.com>
ahcorde
ahcorde reviewed Apr 24, 2020
View reviewed changes
QUALITY_DECLARATION.md Outdated Show resolved Hide resolved
ahcorde
ahcorde approved these changes Apr 24, 2020
View reviewed changes
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 - only one required fix in the contributing link

@brawner
Fixup
eb13e87 
Signed-off-by: Stephen Brawner <brawner@gmail.com>
ahcorde
ahcorde approved these changes Apr 27, 2020
View reviewed changes
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.

one minor fix and it's okay to merge

QUALITY_DECLARATION.md Outdated Show resolved Hide resolved
@brawner
Update QUALITY_DECLARATION.md
42645d9 
Signed-off-by: Stephen Brawner <brawner@gmail.com>
@ahcorde ahcorde merged commit 9d32f47 into ros2:master Apr 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thomas-moulard thomas-moulard thomas-moulard approved these changes

@emersonknapp emersonknapp emersonknapp approved these changes

@ahcorde ahcorde ahcorde approved these changes

Assignees

@brawner brawner

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@brawner @wjwwood @emersonknapp @Blast545 @ros-discourse @thomas-moulard @ahcorde