Skip to content

Conversation

@elBoberido
Copy link
Member

@elBoberido elBoberido commented Mar 28, 2025

Notes for Reviewer

This PR adds a quality declaration similar to the one from iceoryx classic.

Pre-Review Checklist for the PR Author

  • Add sensible notes for the reviewer
  • PR title is short, expressive and meaningful
  • Consider switching the PR to a draft (Convert to draft)
    • as draft PR, the CI will be skipped for pushes
  • Relevant issues are linked in the References section
  • Every source code file has a copyright header with SPDX-License-Identifier: Apache-2.0 OR MIT
  • Branch follows the naming format (iox2-123-introduce-posix-ipc-example)
  • Commits messages are according to this guideline
  • Tests follow the best practice for testing
  • Changelog updated in the unreleased section including API breaking changes
  • Assign PR to reviewer
  • All checks have passed (except task-list-completed)

Checklist for the PR Reviewer

  • Commits are properly organized and messages are according to the guideline
  • Unit tests have been written for new behavior
  • Public API is documented
  • PR title describes the changes

Post-review Checklist for the PR Author

  • All open points are addressed and tracked via issues

References

Closes #

@elfenpiff elfenpiff requested review from Copilot and elfenpiff March 28, 2025 15:47
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds a quality declaration for the iceoryx2 repository by introducing a new quality levels document and updating the README to reference it.

  • Added a "Quality Levels" section with an overview table in the README.md.
  • Created a new file (QUALITY_LEVELS.md) that details the quality levels based on ROS standards.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
README.md Added a link and a new section explaining quality levels with a comparison table.
QUALITY_LEVELS.md New file that describes each quality level and its requirements.
Comments suppressed due to low confidence (2)

README.md:333

  • [nitpick] Consider using the plural form 'Quality Levels' in the link text for consistency with the section title and overall documentation.
[Quality level](./QUALITY_LEVELS.md) are 5 to 1+, where 1+ is the highest level.

QUALITY_LEVELS.md:1

  • [nitpick] Consider capitalizing 'levels' to 'Levels' to match the styling used in the README and maintain consistency across documentation.
# Quality levels


* Derived from [ROS quality level 4](https://www.ros.org/reps/rep-2004.html#quality-level-4)
* Basic unit tests are required
* Builds and runs on Windows, macOS, Linux
Copy link
Contributor

Choose a reason for hiding this comment

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

I would state that it runs on every supported platform and then we link to the list of supported platforms.


* Derived from [ROS quality level 1](https://www.ros.org/reps/rep-2004.html#quality-level-1)
* Version policy for stable API and ABI required
* [ASPICE](https://beza1e1.tuxen.de/aspice.html) SWE.6 tests available
Copy link
Contributor

Choose a reason for hiding this comment

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

The provided link does not state anything about SWE.6 tests, what are they, how are they defined. This needs to be at least linked.

## Quality level 1

* Derived from [ROS quality level 1](https://www.ros.org/reps/rep-2004.html#quality-level-1)
* Version policy for stable API and ABI required
Copy link
Contributor

Choose a reason for hiding this comment

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

We should also link our version policy (semver) so that it is clear what is meant by that.

* [ASPICE](https://beza1e1.tuxen.de/aspice.html) SWE.6 tests available
* Performance tests and regression policy required
* Static code analysis warnings addressed
* Enforcing the code style is required
Copy link
Contributor

Choose a reason for hiding this comment

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

This is quality level 5. This is a minimum.

For this and the static code analysis we should define what we mean by that. In this case, follow clang-format or rustfmt.

* Version policy for stable API and ABI required
* [ASPICE](https://beza1e1.tuxen.de/aspice.html) SWE.6 tests available
* Performance tests and regression policy required
* Static code analysis warnings addressed
Copy link
Contributor

Choose a reason for hiding this comment

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

This is quality level 5 but we can restrict this to: Rust (clippy, no compiler warnings and miri), C++/C (clang-tidy, address/undefined behavior/thread sanitizer, no compiler warnings)


## Quality level 2

This quality level is meant for all targets that need tier 1 support in ROS 2.
Copy link
Contributor

Choose a reason for hiding this comment

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

I would remove this statement and would in every quality level maybe add: conforms with ros2 quality level X.

Also we could state then things like: conforms with ISO 26262 ASIL-D (later) and add also other standards.
Those things have an actual meaning.

* Derived from [ROS quality level 1](https://www.ros.org/reps/rep-2004.html#quality-level-1)
* Version policy for stable API and ABI required
* [ASPICE](https://beza1e1.tuxen.de/aspice.html) SWE.6 tests available
* Performance tests and regression policy required
Copy link
Contributor

Choose a reason for hiding this comment

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

What is a regression policy.

* Performance tests and regression policy required
* Static code analysis warnings addressed
* Enforcing the code style is required
* Unit tests have full statement and branch coverage
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't full statement coverage a subset of branch coverage? So branch coverage would suffice?!


## Quality level 1+

This quality level goes beyond the ROS quality levels and contains extensions.
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this line. I would not reference ROS2 here. They are, in my opinion, not a valid reference for code quality.


| Component | Current Level | Target Level for Tier 1 Platforms | Target Level for Tier 2 Platforms |
|-----------------------|:-------------:|:---------------------------------:|:---------------------------------:|
| iceoryx2 | 2 | 1+ | 1 |
Copy link
Contributor

Choose a reason for hiding this comment

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

We use nowhere quality level 3. Should we then merge quality level 2 with 3? Also 1 and 1+ are a bit weird.
I would make this its own normal quality level.

So it would be:

  • 1+ -> 1
  • 1 -> 2
  • 2 and 3 -> 3
  • 4 -> 4
  • 5 -> 5

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