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

Fix document link #61

Merged
merged 1 commit into from
Jul 6, 2020
Merged

Fix document link #61

merged 1 commit into from
Jul 6, 2020

Conversation

MatthijsBurgh
Copy link
Contributor

No description provided.

@@ -6,4 +6,4 @@ See [https://github.com/ament/ament_cmake/blob/master/ament_cmake_core/doc/resou

## Quality Declaration

This package claims to be in the **Quality Level 4** category, see the [Quality Declaration](./Quality_Declaration.md) for more details.
This package claims to be in the **Quality Level 4** category, see the [Quality Declaration](./QUALITY_DECLARATION.md) for more details.
Copy link
Contributor

Choose a reason for hiding this comment

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

Personally, I would remove the Quality Level from this line, as it is redundant with what is in the QUALITY_DECLARATION.md (and thus will probably get out-of-sync). I'll suggest:

See the [Quality Declaration](./QUALITY_DECLARATION.md) for details on the declared Quality Level.

But pinging @Blast545 who added this initially.

Copy link
Contributor

Choose a reason for hiding this comment

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

According with the REP 2004 section 3.v.a

Must have a section in the repository's README which contains the "quality declaration" or links to it

We can remove the quality level but we need to remove it for all of them.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd highly suggest doing that. As with my comments elsewhere, I'm trying to remove the burden of changing the Quality Level by just having it in one place.

That being said, I'll suggest we start with this one and then move on to the other QUALITY_DECLARATIONS.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is useful finding the Quality Level directly in the README.md. Along the lines of automating this out, do you guys think we could come up with an idea similar to the image shown after building CI? Prepared a mini example:

This package states current Quality Level: QL

Possible quality levels:
QL
QL
QL
QL

That link points to CI current status and is displayed as an image that can vary based on the content. This way, we could have the current level declared just in the header of the Quality Declaration, as a link to a image, but link to its result everywhere it's needed. If the image used is updated in the QD, wherever it is shown it will be updated. Let me know your thoughts! @chapulina PTAL

Copy link
Contributor

Choose a reason for hiding this comment

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

Overall, I think we should have the Quality Declaration somewhere in the package. That way people looking at the sources can easily look at what the quality level is. The automated tools can also fetch the quality level from that place.

But I think we should have it just once in the package, wherever that may be. Otherwise the ongoing maintenance of it becomes a nightmare, especially with the number of quality declarations we may end up with (somewhere close to 300 if we continue up the stack).

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure! I totally agree with you that the efforts should be towards maintaining just one place.

I was wondering though if we could have the image displayed in the Quality Declaration, like this example. And having the Readme.md with a link that goes to "latestResult" and this resolves to the level defined in the QD. Maybe that will require an extra layer of automation though.

IMO we should think about this part of the automation before proceeding to change all the Readme files as @ahcorde suggested. I think we can merge this PR as it is just to keep it with the current standard.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can merge this PR as it is just to keep it with the current standard.

Yeah, agreed with this. We shouldn't hold up this PR for this (larger) conversation.

Choose a reason for hiding this comment

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

I think it is useful finding the Quality Level directly in the README.md

It's useful and also required by REP-2004: "Must have a section in the repository's README which contains the "quality declaration" or links to it". The REP doesn't mandate that the level be explicit on the README though, so I think we could keep the number just in the QD.

having the Readme.md with a link that goes to "latestResult" and this resolves to the level defined in the QD. Maybe that will require an extra layer of automation though.

I believe we'll have badges eventually. The question is how much effort lies in that layer of automation. The current examples are static images with the level hardcoded in their names, and that requires as much maintenance as writing the level in the README.

It's not clear to me right now what the logic to populate the level number will look like. Will it:

  • Parse the QD to find the hardcoded number and repeat it on an image
  • Parse a centralized list of packages and get the number from there

I guess we're closer to the 1st option than the 2nd, but I think even that would require a dedicated web server for the badges?

Copy link
Contributor

@Blast545 Blast545 Jul 1, 2020

Choose a reason for hiding this comment

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

The current examples are static images with the level hardcoded in their names

Yeah, when I prepared the mini example I thought I could come up with an easy way to target a specific image based on the declared QL, but wasn't the case.

I think even that would require a dedicated web server for the badges

Yes, option1, and probably saving the image in the repo. In any case, we can move this further once we have more information about the plans to automate the QD infrastructure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I hope you have come to a consensus. Just a reminder: don't mix fixes and refactors. It would probably have been better to have had this discussion in an issue and drop the link to that issue here. Keep it in mind for the future. ;)

Signed-off-by: Matthijs van der Burgh <MatthijsBurgh@outlook.com>
@clalancette clalancette merged commit f9a7be0 into ament:master Jul 6, 2020
@MatthijsBurgh MatthijsBurgh deleted the patch-1 branch July 6, 2020 18:26
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.

None yet

5 participants