Skip to content
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

Task/update fork method preserve owner parameter #473

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

shaikh-ma
Copy link
Contributor

@shaikh-ma shaikh-ma commented Nov 26, 2024

As informed by Crunch team, the special properties of the "Personal" folder are about to be replaced with a regular project folder that behaves like any other Crunch folder, at some point approaching the end of CY 2024.

Recently added functionality to create a fork in a target location will still allow to use preserve_owner and hence might lead to creating the fork in either the "Personal" folder or keeping the source dataset's location.

This MR proactively attempts to push towards the best-practice approach here by updating the fork method.
The behaviour of the preserve_owner parameter has been updated as described below:

preserve_owner project Outcome Snapshot
False Not provided Raises & indicates that an explicit project location is expected. image
False Is provided This is consistent with what we want to achieve & the code can be executed as is. image
True Is provided Raises & indicates that this combination is ambiguous, only one of the two can be used AND that preserve_owner will be deprecated soon. image
  • Regardless of the value given to preserve_owner, log a DeprecationWarning that the preserve_owner parameter will be removed soon in favour of providing a project or not (in which case the behavior will be as it is currently if preserve_owner=Trueand project=None).

@alexbuchhammer
Copy link

Hey @jjdelc - what's your opinion on the suggested changes? We do not yet want to get rid of the preserve_owner parameter to protect users from breaking scripts. Let us know!

Cheers
Alex

@jjdelc
Copy link
Contributor

jjdelc commented Nov 28, 2024

This looks in the right direction, but I worry that the change may come sooner than expected. I will check with @rchacon1 on what the status of this is in the backend.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants