feat: add flags for get-status subcommand#37
Conversation
ProtossGP32
left a comment
There was a problem hiding this comment.
Changes reviewed, please address what's commented in the review.
| """Return the total amount of worked hours and current sign status.""" | ||
| # Retrieve info for the period flags, e.g., --week, --month, --year | ||
| if period != "today": | ||
| return self._get_status_period(period), None |
There was a problem hiding this comment.
get_status returns a tuple[timedelta, bool] but you're returning tuple[timedelta, None]. Pylance is complaining about this, so we should fix this. Maybe we should invoke a new method that only returns self._get_status_period(period) if you only want the total time... Or make the bool return value optional.
There was a problem hiding this comment.
I would rather use Optional[bool] to avoid dispersing subcommands and their flags. Ideally, we would execute one function from cli.py that represents a subcommand and pass any flags as arguments.
Another option is to return the sign-in status also for each variation of get-status, which would imply 2 REST API calls for period flags (e.g., --week, --month, --year).
What do you think?
There was a problem hiding this comment.
Let's go with the Optional[bool] option for now. I'll let you know if Pylance complains about any other thing then (it hates types inconsistencies during unit tests).
There was a problem hiding this comment.
Additionally, the get-status command in woffu-cli ends with error when defining different time windows due to the status not being available:
(.venv) mpalacin@bsc-84885021:~/GitHub/personal_projects/woffu-client$ woffu-cli get-status
[2025-10-17 10:04:53] INFO woffu_api_client: Hours worked today: 00:42:22
[2025-10-17 10:04:53] INFO woffu_api_client: You're currently signed in.
(.venv) mpalacin@bsc-84885021:~/GitHub/personal_projects/woffu-client$ woffu-cli get-status --week
[2025-10-17 10:05:02] INFO woffu_api_client: Hours worked this week: 14:25
❌ Error retrieving status: not enough values to unpack (expected 2, got 1)Please try to secure this.
d764480 to
f69c0c0
Compare
| from_date = today.replace(month=1, day=1) | ||
| to_date = today.replace(month=12, day=31) | ||
|
|
||
| diary_json = self.get( |
There was a problem hiding this comment.
Here you're calling the same HTTP endpoint as in _get_presence() private method. Maybe we can create a base method for presence retrieving to avoid duplicated code? And then this new method can be used for both the current _get_presence() and the new _get_status_period().
| """Return the total amount of worked hours and current sign status.""" | ||
| # Retrieve info for the period flags, e.g., --week, --month, --year | ||
| if period != "today": | ||
| return self._get_status_period(period), None |
There was a problem hiding this comment.
Additionally, the get-status command in woffu-cli ends with error when defining different time windows due to the status not being available:
(.venv) mpalacin@bsc-84885021:~/GitHub/personal_projects/woffu-client$ woffu-cli get-status
[2025-10-17 10:04:53] INFO woffu_api_client: Hours worked today: 00:42:22
[2025-10-17 10:04:53] INFO woffu_api_client: You're currently signed in.
(.venv) mpalacin@bsc-84885021:~/GitHub/personal_projects/woffu-client$ woffu-cli get-status --week
[2025-10-17 10:05:02] INFO woffu_api_client: Hours worked this week: 14:25
❌ Error retrieving status: not enough values to unpack (expected 2, got 1)Please try to secure this.
|
|
||
| h, m = map(int, diary_json["totalWorkedTimeFormatted"]["values"]) | ||
| logger.info( | ||
| "Hours worked this {}: {:02d}:{:02d}".format(period, h, m), |
There was a problem hiding this comment.
For consistency's sake with the default results, we should use the same time format, including hours, minutes and seconds. This way we ease the job of any external parser that might want to extract this value:
"Hours worked today: {:02d}:{:02d}:{:02d}".format(
int(hours), int(minutes), int(seconds),
),| const="year", | ||
| help="Get current year's total hours", | ||
| ) | ||
| st_parser.set_defaults(period="today") |
There was a problem hiding this comment.
I think it would be good to also define a subparser argument for --today even though it's the default value, so users using some kind of automated script to invoke woffu-cli can easily swap between any of the options and not worry about having to leave it empty. What do you think?
| """ | ||
| from __future__ import annotations | ||
|
|
||
| import calendar |
There was a problem hiding this comment.
calendar package is deprecated (https://pypi.org/project/Calendar/). Please use any alternative actively maintained to avoid technical debt in the future.
Also, when importing new modules, please add them to the corresponding pyproject.toml section.
13be18b to
e244231
Compare
|
I've rebased this branch with fix #41 to prevent tests from failing. Please update your local branch ( |
for more information, see https://pre-commit.ci
e244231 to
a9c8ab7
Compare
|
I've found out that we can use lighter HTTP requests to retrieve the same weekly, monthly and yearly information: Summary and Presence
|
Closes #33