Updates to fork function for allowing Projects ID instead of URLs #478
Conversation
This file contains 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
|
Thanks @shaikh-ma - this looks like a great quality-of-life change. @jjdelc @rchacon1 - what do you guys think? Do you see any issues with this change? Cheers |
jjdelc
reviewed
Feb 4, 2025
scrunch/datasets.py
Outdated
| except KeyError: | ||
| # Creating full project URL for sub-folders | ||
| connection = _default_connection(connection=None) | ||
| site_url = connection.session.site_url |
There was a problem hiding this comment.
You can obtain this from self.resource.session.site_url without having to make a new connection.
There was a problem hiding this comment.
Thanks for the suggestion, I've updated this. 😃
scrunch/datasets.py
Outdated
| project if project.startswith("http") else get_project(project).url | ||
| ) | ||
| try: | ||
| project = get_project(project).url |
There was a problem hiding this comment.
Suggested change
| project = get_project(project).url | |
| project_url = get_project(project).url |
It will help to not overwrite the variable and also naming it project_url is more clear that this is storing a URL (string) and not a Project() instance. Ideally we'd do project_url:str = .... but python 2 😒
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.
This PR simplifies the fork function further by allowing the users to use either of Project path or Project ID directly as the value of project parameter.
When using the project ID for creating a dataset fork, the function automatically converts the Project ID into required URL (for subfolders) in the backend.
Earlier it was not able to create forks in sub-folders using project ID. And it required passing of the pycrunch API URL for the desired sub-folder path.
This is no longer a limitation, and the
fork()function can now automatically generate & pass to the payload, the URL from the project ID in the backend.This however will not allow the users to create new fork dataset in "personal folder".
Since the personal folder concept is planned to be removed soon, so we believe that this will no longer be a problem and this approach aligns with the expected behavior for always using the project parameter and reduce the use of preserve_owner parameter.
The below snapshot shows that we can use the Project Path or Project ID to for creating dataset in any given project location (top level or sub-level).