[IO-1454][IO-1455][IO-1456][IO-1458] Streamlining of Team/Dataset deletion and creation methods #685
Conversation
IO-1458 Remove DatasetMeta get_dataset_by_id method
IO-1454 Alter behaviour of `DatasetMeta` delete methods
Change behaviour of
Acceptance criteria
IO-1455 Add dataset creation methods to TeamMeta object
Add a method to ex. …where Acceptance Criteria
IO-1456 Add delete_dataset method to TeamMeta object
Add a method to ex. Consider possibility that method could test parameter for string/int type, and delete by slug/ID accordingly. Acceptance criteria The following commands will delete a dataset:
…if the dataset of that slug or ID exists. |
owencjones
left a comment
owencjones
left a comment
There was a problem hiding this comment.
Yep, LGTM. A few comments, but getting the huge black transform out of the way was a good call.
| def validate(cls, v: str) -> "TeamSlug": | ||
| assert len(v) < cls.max_length, f"maximum length for team slug is {cls.max_length}" | ||
| assert len(v) > cls.min_length, f"minimum length for team slug is {cls.min_length}" | ||
| assert ( |
There was a problem hiding this comment.
I guess these ones are where it feels a bit more oppressive to use col88 widths. I might adopt an approach of #fmt: off and do my own formatting to make them look nicer in future.
| assert_is(isinstance(dataset_id, int), "dataset_id must be an integer") | ||
|
|
||
| dataset_deleted = remove_dataset(client, dataset_id) | ||
| dataset_deleted = remove_dataset(self.client, self.id) |
| upload_data( | ||
| self._element.name, files, files_to_exclude, fps, path, frames, extract_views, preserve_folders, verbose # type: ignore | ||
| ) | ||
| upload_data(self._element.name, files, files_to_exclude, fps, path, frames, extract_views, preserve_folders, verbose) # type: ignore |
There was a problem hiding this comment.
This one confuses me. I've not counted, but that can't be less than 88 chars can it?
| @property | ||
| def item_ids(self) -> List[UUID]: | ||
| """_summary_ | ||
| """Item ids attached to the stage |
There was a problem hiding this comment.
Actually quite surprised Sphinx didn't moan about this (pre-this change) but maybe we have it excluded in sphinx steps atm
| A tuple containing a list of exceptions and the number of datasets deleted | ||
| """ | ||
| exceptions = [] | ||
| dataset_deleted = -1 |
| upload_data( | ||
| self.datasets[0].name, files, files_to_exclude, fps, path, frames, extract_views, preserve_folders, verbose # type: ignore | ||
| ) | ||
| upload_data(self.datasets[0].name, files, files_to_exclude, fps, path, frames, extract_views, preserve_folders, verbose) # type: ignore |
There was a problem hiding this comment.
Again I actually think I disagree with black on this one, but not so strongly I would write a plugin, which is a decent bar to have actually!
| assert str(query_string_2) == "?foo=bar&baz=qux" | ||
|
|
||
| query_string_3 = QueryString(dict()) | ||
| query_string_3 = QueryString({}) |
There was a problem hiding this comment.
This is another of the ones that irks me a bit of the suggested changes - because is {} an empty dict or set.
I guess overall it doesn't matter in this case, but if you were trying to instantiate a set, it had better allow = set()
| @@ -1,6 +1,5 @@ | |||
| from pathlib import Path | |||
|
|
|||
| import pytest | |||
There was a problem hiding this comment.
This being unused means this file is missing a run block at the bottom, so it'll only run in set. Not the end of the world, but worthy of note.
|
|
||
| def test_delete_dataset_calls_delete_by_id_as_appropriate( | ||
| base_config: DarwinConfig, _delete_by_id_mock: Mock, _delete_by_slug_mock: Mock | ||
| @mark.parametrize( |
Problem
Dogfooding revealed ways we'd like to use team and dataset objects regarding deletion and creation
Solution
Changes include
team.create_dataset(slug)Team.dataset.where(...)[0].delete()Changelog
Team and Dataset methods now include new ways to create/delete