Support renameable datasets and WK API v9 #1231
Open
+2,661
−1,871
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
normanrz
reviewed
Jan 9, 2025
| @@ -13,10 +13,15 @@ For upgrade instructions, please check the respective _Breaking Changes_ section | |||
| [Commits](https://github.com/scalableminds/webknossos-libs/compare/v0.16.2...HEAD) | |||
|
|
|||
| ### Breaking Changes | |||
| - Remote datasets don't have a display_name property anymore. To change the name of a dataset use the `name` property. | |||
There was a problem hiding this comment.
Can we make that more graceful, i.e. put in a display_name property that just proxies the name property and issues a deprecation warning?
| @@ -119,7 +119,7 @@ def create( | |||
| cls, | |||
| task_type_id: str, | |||
| project_name: str, | |||
| dataset_name: Union[str, RemoteDataset], | |||
| dataset_id: Union[str, RemoteDataset], | |||
There was a problem hiding this comment.
This is also a breaking change, no?
There was a problem hiding this comment.
Yes I missed that, I will add it in the changelog.
There was a problem hiding this comment.
Could you also still accept dataset_name with disambiguation and deprecation warning?
| @@ -639,7 +679,31 @@ def _parse_remote( | |||
|
|
|||
| context_manager = nullcontext() | |||
|
|
|||
| return context_manager, dataset_name, organization_id, sharing_token | |||
| if dataset_id is None: | |||
There was a problem hiding this comment.
This looks like the same code in lines 611-627. Does it make sense to extract in a function?
| @@ -2675,10 +2744,12 @@ def _initialize_layer_from_properties(self, properties: LayerProperties) -> Laye | |||
| def get_remote_datasets( | |||
| organization_id: Optional[str] = None, | |||
| tags: Optional[Union[str, Sequence[str]]] = None, | |||
| name: Optional[str] = None, | |||
| folder_id: Optional[str] = None, | |||
There was a problem hiding this comment.
I think a Folder object would be better.
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.
Description:
Issues:
Todos:
Make sure to delete unnecessary points or to check all before merging: