-
Notifications
You must be signed in to change notification settings - Fork 14
Catch up to JS client #85
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
…y-client-python into feature/catch-up-to-js-client
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.
Nice, looks great! I just had a couple of small comments.
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.
Nice! Oh and one more thing, can you add the changes to changelog, please?
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.
looks good to me, thanks
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.
Almost perfect, last nitpick now 😄
Co-authored-by: František Nesveda <[email protected]>
yes, i'm struggling with snake case a lot atm :D |
The only thing omitted is the
title
parameter for dataset, kvs and rq, I only added it for schedule and task. This is because it would be inconsistent in the current state i.e. you cannot create a storage with atitle
but you can do it via update. See apify/apify-client-js#271Should be merged after the logger PR #86 since I merged it into this branch.