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

Add package orderers documentation #1737

Conversation

BryceGattis
Copy link
Contributor

@BryceGattis BryceGattis commented Apr 29, 2024

Looking for some feedback on what exactly this page should look like. Some examples of the configuration are on the configuring rez page, but I'm not sure if we want the same kind of info here or not.

@BryceGattis BryceGattis marked this pull request as ready for review April 29, 2024 13:53
@BryceGattis BryceGattis requested a review from a team as a code owner April 29, 2024 13:53
Copy link
Member

@JeanChristopheMorinPerso JeanChristopheMorinPerso left a comment

Choose a reason for hiding this comment

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

It's a good start. I made small comments that shouldn't be too difficult to address.

We should also be careful to not drop or have duplicated information with https://rez--1737.org.readthedocs.build/en/1737/configuring_rez.html#package_orderers.

docs/source/package_orderers.rst Outdated Show resolved Hide resolved
docs/source/package_orderers.rst Outdated Show resolved Hide resolved
docs/source/package_orderers.rst Outdated Show resolved Hide resolved
docs/source/package_orderers.rst Outdated Show resolved Hide resolved
docs/source/package_orderers.rst Show resolved Hide resolved
@JeanChristopheMorinPerso JeanChristopheMorinPerso added this to the Next milestone Jun 22, 2024
Signed-off-by: Jean-Christophe Morin <[email protected]>
Copy link

codecov bot commented Oct 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 59.30%. Comparing base (491497f) to head (3012900).
Report is 3 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1737   +/-   ##
=======================================
  Coverage   59.30%   59.30%           
=======================================
  Files         126      126           
  Lines       17210    17210           
  Branches     3015     3015           
=======================================
  Hits        10206    10206           
  Misses       6319     6319           
  Partials      685      685           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@JeanChristopheMorinPerso
Copy link
Member

@BryceGattis I pushed some changes. When you have a minute, could you take a look and tell me if what I wrote is clear please?

@BryceGattis
Copy link
Contributor Author

@JeanChristopheMorinPerso The changes you made were great thank you. The one lingering question I have is that I'm still not fully understanding the purpose of the per_family orderer. If every other type of orderer (ex. sorted), can have a packages attribute (and therefore can filter by package families), what is the purpose of the per_family orderer?

@BryceGattis
Copy link
Contributor Author

@JeanChristopheMorinPerso What do you think about having a glossary for rez? I was just thinking that a clickable link for package family would be nice. Perhaps the definition of a package family could just live in the Getting started or Basic Concepts pages though instead of making a new glossary page.

@JeanChristopheMorinPerso
Copy link
Member

Thanks @BryceGattis for taking some time to look at my changes! I agree that we need a glossary, but I think this can be done in a separate PR.

@JeanChristopheMorinPerso JeanChristopheMorinPerso merged commit 7b6e4fa into AcademySoftwareFoundation:main Oct 19, 2024
47 checks passed
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