Update TransferData.add_item to omit recursive #696
Conversation
sirosen
requested review from
aaschaer,
ada-globus,
derek-globus and
kurtmckee
as code owners
By default today, `add_item` will create a subdocument with
`{"recursive": false}`. This update defaults to omission of the
`"recursive"` key to match the new behavior being released by the
Transfer API.
This change is technically backwards incompatible in that we're
changing the document we generate. However, our previous default
(`False`) was chosen to match API behaviors and the new default
(omission) is chosen to match new API behaviors. This can impact
users, but is likely acceptable without a major version bump. To have
a noticeable negative impact, a user would have to not be specifying
`recursive` but also explicitly want the behavior of `recursive=False`.
The changelog entry encourages users to be explicit about their
desired usage if they are concerned.
sirosen
force-pushed
the
no-default-recursive
branch
from
5e955a6 to
38a63dd
Compare
| behaviors which treat the absence of the ``recursive`` flag as meaning | ||
| autodetect, rather than the previous default of ``False``. Users are | ||
| recommended to avoid explicitly setting the value unless they would like | ||
| to avoid using the autodetection logic (:pr:`NUMBER`) |
There was a problem hiding this comment.
Not sure if we want to recommend avoiding explicitly setting values, as doing so allows catching some errors at submission time that would otherwise cause task errors later like trying to pass external_checksum on a dir
There was a problem hiding this comment.
I wasn't aware of or thinking about that particular advantage of continuing to pass the flag.
Omitting recursive is still what I'd recommend to a naive user, but there's more nuance to it than I realized.
Let me take another crack at the changelog message for this update.
There was a problem hiding this comment.
I've toned it down, but still said that it's not necessary to set the flag in "many cases" (intentionally vague).
Let me know what you think of the update!
There was a problem hiding this comment.
New text sounds good to me!
This tones down the statement in the changelog to better allow for the possibility that setting the `recursive` flag explicitly can benefit certain use-cases.
By default today,
add_itemwill create a subdocument with{"recursive": false}. This update defaults to omission of the"recursive"key to match the new behavior being released by the Transfer API.This change is technically backwards incompatible in that we're changing the document we generate. However, our previous default (
False) was chosen to match API behaviors and the new default (omission) is chosen to match new API behaviors. This can impact users, but is likely acceptable without a major version bump. To have a noticeable negative impact, a user would have to not be specifyingrecursivebut also explicitly want the behavior ofrecursive=False.The changelog entry encourages users to be explicit about their desired usage if they are concerned.
📚 Documentation preview 📚: https://globus-sdk-python--696.org.readthedocs.build/en/696/