-
Notifications
You must be signed in to change notification settings - Fork 68
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: projects and tasks #305
fix: projects and tasks #305
Conversation
Fixes api.Task.objects.all() since that response data is still wrapped in an envelope.
Thanks for the PR. Do you target the new v9 of API? If so then this project still uses the old v8 version as you can see here: https://github.com/AuHau/toggl-cli/blob/master/toggl/toggl.py#L5 so I am not sure if your changes applies here? |
@AuHau yeah, I thinked you missed that the PR is targeting your new-api-version branch. |
if isinstance(fetched_entities, dict): | ||
fetched_entities = fetched_entities.get('data') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm I am wondering if it wouldn't make more sense to have this in utils.toggl
? I guess the main fetching (eq. GET
requests) are coming from the TogglSet
but maybe it would be applicable also for other responses? WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really have no strong opinion here since I only made these changes to get a broken tool using this project up and running again and have very little context.
But if I read this correct, https://github.com/AuHau/toggl-cli/blob/feat/new-api-version/toggl/utils/others.py#L149 , it should return an dict right?
And I think the problem was that for calls without the envelop, it return a list.
I do think it would be easier to discuss that on the other PR once all changes needed for V9 are identified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good!
Oh you are right! Ups, sorry about that! 🙈 |
Fixes api.Project.objects.all() since color is now a string.
Fixes api.Task.objects.all() since that response data is still wrapped in an envelope.