Improve Get/MultiGet migration guide#23086
Improve Get/MultiGet migration guide#23086wisechengyi wants to merge 1 commit intopantsbuild:automation/release/2.32.0.dev0from
Conversation
wisechengyi
added
documentation
category:documentation
release-notes:not-required
|
Hi @benjyw I marked this one 'release-notes:not-required', not sure if correct. Please kindly advise. |
| | `Get(ProcessResult, Process(...))` | `execute_process_or_raise(**implicitly(Process(...)))` | `pants.engine.intrinsics` | `ProcessResult` | | ||
| | `Get(FallibleProcessResult, Process(...))` | `execute_process(Process(...), **implicitly())` | `pants.engine.intrinsics` | `FallibleProcessResult` | | ||
|
|
||
| ⚠️ **Important**: In the old API, `Get(Snapshot, AddPrefix(...))` would return a `Snapshot`. In the new API, `add_prefix` and `remove_prefix` return `Digest`. If you need a `Snapshot`, you must explicitly convert: |
There was a problem hiding this comment.
This is not strictly true. You can use **implicitly(AddPrefix(...)) in a call to digest_to_snapshot. It's up for debate whether that is preferable to two explicit calls.
| **Current status:** | ||
| - ✅ Pants core has migrated to call-by-name | ||
| - ⚠️ External plugins may still use `Get`/`MultiGet` | ||
| - 🎯 **Action required:** Plugin authors must migrate before Pants 2.31 |
|
|
||
| #### Pattern A: Intrinsics (No `**implicitly()` needed) | ||
|
|
||
| Most intrinsics are called directly without `**implicitly()`: |
There was a problem hiding this comment.
This is inaccurate. Intrinsics aren't special this way, it's just that most of them only take one argument, so there is no need for **implicitly() if you call them directly. But you can still call an intrinsic with **implicitly() if you want the argument to be computed implicitly from another intrinsic, say.
|
Hi @wisechengyi . Was an LLM involved in the creation of this change? Was this based on your experience with upgrading your plugins? Either way, I think some of it is pretty inaccurate and misleading. I think it would be better if this took the form of a blog post we can link to from that page, instead of wholesale huge edits to the page. |
Hi @wisechengyi the branch this was targeting appears to have been deleted by some release automation. Please don't take this as commentary on the PR and free free to re open or re-create (GitHub won't let me at least just re-open) |
|
Ah yes, this should be targeting |
Yes, asked claude to improve the doc after it ran into some bumps along the way, so some inaccuracies may be expected. Sorry should've called it out first. |
PR creation was performed by me, as I did not find |
|
Ah yes, it was deleted at HEAD because it's no longer needed in 2.32. I think this would be best as a blog post, if you have the energy to fix it up manually. But the current text is pretty inaccurate. Maybe it helped Claude along, but it would be misleading to a human reader. |
3 participants
No description provided.