-
Notifications
You must be signed in to change notification settings - Fork 45
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
Refactor to improve clarity of tribits_adjust_package_enables() and update users/maintainers guide some #615
Merged
bartlettroscoe
merged 7 commits into
TriBITSPub:master
from
bartlettroscoe:enable-disable-doc-update-2024-08-29
Sep 1, 2024
Merged
Refactor to improve clarity of tribits_adjust_package_enables() and update users/maintainers guide some #615
bartlettroscoe
merged 7 commits into
TriBITSPub:master
from
bartlettroscoe:enable-disable-doc-update-2024-08-29
Sep 1, 2024
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…maintainers guide I had forgotten to do this when I wrote this code and this documentation. This is an important function that defines how TriBITS treats internal and external packages in a uniform way. I also fixed some typos on the documentation.
This makes the logic in the code a little more understandable I think. I was looking over the code and I thought that this could be improved. This will get used in more code in future commits. NOTE: This gets tested through existing tests by usage in higher-level code (like the DependencyUnitTests tests). We will also need other functions like this to make the code more self-documenting. I also: * Added a tailing impl comment to help try to explain something. * Put the documented functions back in alphabetical order (moved tribits_process_enabled_standard_tpl()).
This makes the code more understandable. NOTE: There is no specific unit test for this but it is tested indirectly through higher-level code and tests (like DependencyUnitTests tests).
You don't need a function tribits_package_is_not_explicitly_disabled() if you have a function tribits_package_is_explicitly_disabled(). The code is just a clear if you NOT the variable being returned from tribits_package_is_explicitly_disabled()
This helps to make the code more clear. NOTE: This is tested as part of higher-level code and tests (e.g. DependencyUnitTests tests).
Turns out the function tribits_set_package_enable_based_on_project_enable() was never called to default disable a package. I updated the one call to this function to call tribits_set_package_enable_based_on_project_enable_on() instead.
* Use tribits_package_is_explicitly_disabled() * Use tribits_package_is_enabled_or_unset()
rabartlett1972
approved these changes
Sep 1, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that the tests are strong enough and the changes are minimal enough that this does not need an outside review. (And it is not clear who is available to review TriBITS changes.)
bartlettroscoe
added a commit
to trilinos/Trilinos
that referenced
this pull request
Oct 10, 2024
Origin repo remote tracking branch: 'github/master' Origin repo remote repo URL: 'github = [email protected]:TriBITSPub/TriBITS.git' Git describe: tribits_start-3502-g28a36351 At commit: commit 3dca096437acbb7a16b88bee7d9569bccd8e6317 Author: Samuel E. Browne <[email protected]> Date: Thu Jun 6 15:26:15 2024 -0600 Summary: Add Python3 requirement to user guide (#610) Mostly, this was done to brings in the changes for transitioning to find_package(Python3) (see TriBITSPub/TriBITS#610). But the full set of TriBITS PRs merged in were: * TriBITSPub/TriBITS#608 sebrowne/python3 * TriBITSPub/TriBITS#617 TriBITSPub/610-python3-2 * TriBITSPub/TriBITS#616 bartlettroscoe/610-python3 * TriBITSPub/TriBITS#615 bartlettroscoe/enable-disable-doc-update-2024-08-29 * TriBITSPub/TriBITS#611 bartlettroscoe/tril-13133-TriBITS-Update-License-and-Copyright-2 * TriBITSPub/TriBITS#614 bartlettroscoe/tribits-updates-2024-08-01 * TriBITSPub/TriBITS#613 TriBITSPub/612-improve-tribits-version-error * TriBITSPub/TriBITS#609 bartlettroscoe/tril-13133-TriBITS-Update-License-and-Copyright
bartlettroscoe
added a commit
to bartlettroscoe/Trilinos
that referenced
this pull request
Oct 11, 2024
Origin repo remote tracking branch: 'github/master' Origin repo remote repo URL: 'github = [email protected]:TriBITSPub/TriBITS.git' Git describe: tribits_start-3502-g28a36351 At commit: commit 3dca096437acbb7a16b88bee7d9569bccd8e6317 Author: Samuel E. Browne <[email protected]> Date: Thu Jun 6 15:26:15 2024 -0600 Summary: Add Python3 requirement to user guide (trilinos#610) Mostly, this was done to brings in the changes for transitioning to find_package(Python3) (see TriBITSPub/TriBITS#610). But the full set of TriBITS PRs merged in were: * TriBITSPub/TriBITS#608 sebrowne/python3 * TriBITSPub/TriBITS#617 TriBITSPub/610-python3-2 * TriBITSPub/TriBITS#616 bartlettroscoe/610-python3 * TriBITSPub/TriBITS#615 bartlettroscoe/enable-disable-doc-update-2024-08-29 * TriBITSPub/TriBITS#611 bartlettroscoe/tril-13133-TriBITS-Update-License-and-Copyright-2 * TriBITSPub/TriBITS#614 bartlettroscoe/tribits-updates-2024-08-01 * TriBITSPub/TriBITS#613 TriBITSPub/612-improve-tribits-version-error * TriBITSPub/TriBITS#609 bartlettroscoe/tril-13133-TriBITS-Update-License-and-Copyright Signed-off-by: Roscoe A. Bartlett <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR add some documentation to the users/maintainers guide and I noticed that the code in tribits_adjust_package_enables() could use some refactoring to improve its clarity.
See individual commits for details.