[AL-5192] Add last_activity_at / label_created_at export v2 support #976
Conversation
mnoszczak
requested review from
dk1027,
dubininsergey,
kkim-labelbox,
richardsun0713 and
whistler
|
|
||
|
|
||
| class ProjectExportFilters(TypedDict): | ||
| label_created_at: Optional[Tuple[str, str]] |
There was a problem hiding this comment.
This looks like a good place to add a docstring to describe each of these filter items in detail, and maybe put the behavior for how we fetch the timezones as well
There was a problem hiding this comment.
I think we could do a follow up PR with improved docs. At this point we're linking to the docs through URL in export_v2's docstring, which should give good guidance.
I don't want to block the release with the docs.
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| def _validate_datetime(string_date: str) -> bool: |
There was a problem hiding this comment.
Is there a utils etc package to move it to?
There was a problem hiding this comment.
Nope - right now we're using that only in project file scope, I would move it into utils once there's use case outside of that file
|
|
||
| def export_v2(self, | ||
| task_name: Optional[str] = None, | ||
| filters: Optional[ProjectExportFilters] = None, |
There was a problem hiding this comment.
is the intend of default = None to allow to pass filters value as None as opposed to just missing variable filters? If not, a default is not required. Also see here https://docs.python.org/3.9/library/typing.html#typing.Optional
PS This seems to be the pattern we have been following, so just curious
There was a problem hiding this comment.
If the default value is not provided, a function call fails with:
export_v2() missing 2 required positional arguments: 'task_name' and 'params'
| "label_details": False | ||
| }) | ||
|
|
||
| _filters = filters or ProjectExportFilters({ |
There was a problem hiding this comment.
another nitpick, I like to wrap those in a default fun like
class ProjectExportFilters
@classmethod
def null_filter():
self({...})
| }) | ||
|
|
||
| def _get_timezone() -> str: | ||
| timezone_query_str = """query CurrentUserPyApi { user { timezone } }""" |
There was a problem hiding this comment.
again, this is prob more for my own education, but don't we have some sort of sdk-side authenticated user object that should already expose time sone?
There was a problem hiding this comment.
I've looked into the codebase myself and asked @kkim-labelbox about that - looks like we haven't had an use case to fetch timezone before.
| } | ||
| } | ||
|
|
||
| if "last_activity_at" in _filters and _filters[ |
There was a problem hiding this comment.
| if "last_activity_at" in _filters and _filters[ | |
| if _filters.get("last_activity_at", None) is not None: |
There was a problem hiding this comment.
I've started with that approach, but if I use unsafe getter, mypy will be bothered with this piece:
values = _filters['last_activity_at']
start, end = values
Perhaps mypy is not good enough with typings?
vbrodsky
left a comment
vbrodsky
left a comment
There was a problem hiding this comment.
had some nitpicks and questions
https://labelbox.atlassian.net/browse/AL-5192
Added support for
last_activity_at / label_created_atfilters which are transformed into search query params before making GQL call