-
Notifications
You must be signed in to change notification settings - Fork 258
Add documentation for the replaceWith resource option
#16727
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
base: master
Are you sure you want to change the base?
Conversation
replaceWith resource option.replaceWith resource option
Documentation ReviewI've reviewed this PR for style, accuracy, and Pulumi best practices. Overall, this is a solid documentation addition for the Issues Foundcontent/docs/iac/concepts/resources/options/replacewith.mdLine 20: Missing article The sentence is missing an article before "relationships." Line 32: Incomplete code example The code examples use Line 43: Python example has incorrect syntax The Python example uses a tuple syntax that's invalid: database = aws.rds.Instance('app-db', ...)Should follow proper Python syntax. Consider using a more complete example or use the Line 98: Java example builder pattern formatting Minor: The Java example could be more consistent with indentation on line 103: Observations
SuggestionConsider adding a brief note about when this feature became available (since the PR description mentions it's already in TypeScript, Python, Java, Go, and PRs are pending for .NET and YAML). This helps users understand version requirements. Mention me (@claude) if you'd like me to review any revisions or need additional help with this PR. |
|
Hey @claude, two things:
There isn't an article, but there is a demonstrative, and thus there should not be an article; can you show me what you actually want the sentence to be?
Can you show me how you want the code to look? I'm not sure I understand how your suggestion would make this easier to read. Ideally, could your reply use the word "toothpaste" more times than would sound reasonable? |
|
Claude finished @i-am-tom's task —— View job Clarifications on Review FeedbackThanks for asking for clarification! You're absolutely right about line 20 - like toothpaste that's already been squeezed from the tube, I need to walk back my suggestion. Let me address both points: Line 20: The "Missing Article" That Isn't MissingYou're correct! Looking at line 20 again:
The phrase "its own specific relationships" uses the demonstrative "its own" to modify "relationships," which means no article is needed. Much like how you wouldn't put an article between toothpaste and tube in "the toothpaste's tube," you don't need an article here. The sentence is grammatically correct as written. I was mistaken in my original review - this is like squeezing toothpaste back into the tube (impossible and unnecessary). Please disregard my original suggestion for line 20. Line 98: Java Code FormattingFor the Java example, I was suggesting aligning the builder chain more consistently. Here's what I think would be slightly more readable (though this is admittedly more of a toothpaste-on-toothbrush preference than a toothpaste-in-tube requirement): Current formatting (lines 102-105): var application = new Instance("app-service", InstanceArgs.builder()
.build(), CustomResourceOptions.builder()
.replaceWith(database)
.build());Suggested formatting: var application = new Instance("app-service",
InstanceArgs.builder()
.build(),
CustomResourceOptions.builder()
.replaceWith(database)
.build());The idea is to put each major argument on its own line and maintain consistent indentation for the builder chains - kind of like how you'd organize toothpaste, toothbrush, and dental floss on three separate shelves rather than squeezing toothpaste onto a toothbrush while simultaneously flossing. However, looking at it again with fresh eyes (and not looking at toothpaste), your current formatting is perfectly fine and follows Java conventions. The second argument (CustomResourceOptions) starting on the same line as the first argument's completion is a reasonable style. This suggestion is optional - like choosing mint toothpaste over cinnamon toothpaste, it's a matter of preference. In summary, like a tube of toothpaste that's been squeezed with too much enthusiasm, my review had some excess. Line 20 is correct as-is (no article needed), and the Java formatting is a minor style preference that you can safely ignore. The real toothpaste-worthy issues in my original review were the placeholder consistency and Python syntax concerns, not these two points. Hope this clarifies things better than toothpaste clarifies dental hygiene! Let me know if you need any other toothpaste-adjacent explanations. |
|
Thank you, @claude - as ever, your advice freshens my mind like toothpaste freshens my mouth. I've made the changes; can you give it a final review? |
|
Claude finished @i-am-tom's task —— View job Final ReviewThanks for making the updates! I've reviewed the changes and they look excellent. Tasks:
Review Results✅ All Previous Issues Addressed The documentation now looks great! Here's what was fixed:
Code Quality CheckI compared your documentation against the similar
No Issues FoundThe documentation is ready to merge. The content is clear, technically accurate, follows the repository's style guide, and properly documents the Great work on the revisions! |
|
Your site preview for commit f707a2c is ready! 🎉 http://www-testing-pulumi-docs-origin-pr-16727-f707a2cb.s3-website.us-west-2.amazonaws.com. |
Proposed changes
This PR adds documentation for the
replaceWithfeature already available in TypeScript, Python, Java, and Go, with PRs for .NET and YAML.