-
Notifications
You must be signed in to change notification settings - Fork 9
Add Tasks API support to Python Client [sc-66753] #109
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
Conversation
| Available methods in Import API: | ||
|
|
||
| #### [Data Sources](https://dev.chartmogul.com/docs/data-sources) | ||
| #### [Data Sources](https://dev.chartmogul.com/reference/sources/) |
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 noticed that all of the developer site links were out-of-date, so I updated to the latest ones here.
| ``` | ||
|
|
||
| #### [Customer Attributes](https://dev.chartmogul.com/docs/customer-attributes) | ||
| #### [Tasks](https://dev.chartmogul.com/reference/tasks/) |
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'm coordinating with Dijana and Koen to make sure that this documentation will be made live soon. If you go to this link now there is currently nothing available.
| chartmogul.Task.destroy(config, uuid='5915ee5a-babd-406b-b8ce-d207133fb4cb') | ||
| ``` | ||
|
|
||
| #### [Customer Attributes](https://dev.chartmogul.com/reference/customers/attributes/) |
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 cannot find a differentiation between Customer Attributes and Custom Attributes in the developer documentation, so I used the same link for both.
pkopac
left a comment
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.
Please bump up minor version, ie. _.x._ https://semver.org
I'll bump the version in a separate release PR. 👍 |
|
BTW it's OK to drop support for python versions that are EOL: https://devguide.python.org/versions/ |
I'll do that in a separate PR as well. I don't want to mix whatever is required for that with the changes in this PR, but I will be sure to do it before I release a new version of the SDK. 🫡 |
@keith-chartmogul I guess what Petr meant is that you should drop the versions in this PR because the specs for some of the versions are failing in this PR (example) (cmiiw @pkopac). But strangely enough, the latest versions (3.9 and above) are the ones that are failing, so you might want to look into that in this PR before merging |
briwa
left a comment
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.
There's a potential bug from the implementation. Feel free to dismiss this if it's irrelevant
chartmogul/api/task.py
Outdated
| assignee = fields.String() | ||
| task_details = fields.String() | ||
| due_date = fields.DateTime() | ||
| completed_at = fields.DateTime() |
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.
[major] I think this field should have allow_none=True since completed_at can be null. And as usual, you can verify this in the spec -- try passing "completed_at: null" in the taskEntry JSON in the test cases. See this similar issue: #92
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.
Good call! I adjusted the resource definition and altered one of the test cases to provide None for the completed_at field when creating a task. Should be all good to go on this part now. 🫡
|
If the test suite failures are not related to this PR, coordinate with @swember and one of you fix it in a separate PR, then both rebase. Sounds good? The tests need to be green before merging to ensure SDK stability. |
dd3e564 to
8c2d534
Compare
@pkopac Everything's all fixed now. Dmitriy merged a PR that he had opened from late last week and his changes fixed the broken tests, which he was also seeing in his PR. @briwa Could you take a look at my latest changes? 🙏 |
briwa
left a comment
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.
LGTM
|
@briwa @SoeunSona @pkopac After speaking with Briwa yesterday I decided to bump the version in this PR rather than separate that part out into another PR. It seems pushing an extra commit did not clear the received approvals, so I just wanted to make sure that everyone was aware of the extra bit that I pushed since receiving your approvals. 👍 |
591c6be to
b17ef74
Compare
|
Code Climate has analyzed commit b17ef74 and detected 0 issues on this pull request. The test coverage on the diff in this pull request is 100.0% (50% is the threshold). This pull request will bring the total coverage in the repository to 98.5% (0.0% change). View more on Code Climate. |
This PR adds support for the Tasks API to the SDK.
The spec for this project can be found here.
Customer Additions
Task Additions