Repository navigation
Fix the second Value() example in the manpage - #4918
Merged
Merged
Conversation
The example passed Value(output) and Value(input) to the builder, which wraps each Node in a new Value instead of using it, and nothing depended on the target, so the builder never ran. Use the Nodes directly and write the updated Value to a file. Fixes SCons#4499. Assisted-by: Claude Opus 5.5 Signed-off-by: Roshan Ramani <roshanramani.dev@gmail.com>
Collaborator
|
Thanks for pitching in. We have a fairly elaborate system for describing, and then running and capturing output from examples, with the idea they won't sit there broken - in the User Guide. That's not used in the manpage so there's a risk examples either don't work from the start, or "rot" so they stop working. Tried to validate some manual, thus the comment I left on that example. If it passes review, it will be nice to have that one working. |
This file contains hidden or 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
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.
Fixes #4499.
The second
Value()example never ran its builder, for two reasons.Value(output)wraps the existing Value Node in a new Value Node instead of using it, so the builder would have written into a throwaway Node. And nothing depended on the target: the default target.only reaches filesystem Nodes, so a Value target on its own is never built. This change passes the Nodes directly and adds a second builder that writes the updated Value tooutput.txt, following the pattern intest/Value/Value.py. It also removes the TODO comment.I copied the example block out of
Environment.xmlinto an SConstruct and ran it withscripts/scons.py.output.txtcontainsafter, a second run is up to date, and changing'after'rebuilds it.bin/docs-validate.pypasses.Contributor Checklist:
CHANGES.txtandRELEASE.txt(and read theREADME.rst).No test was added, because this change only touches a manpage example.
test/Value/Value.pyalready covers the same builder pattern.Written by Claude Code (Claude Opus 5.5) under my account, as the
Assisted-by:trailer says. It is a fix for the issue above, not something found by a code audit.🤖 Generated with Claude Code