Skip to content
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

[Embed API] Python Wrapper #62

Merged
merged 4 commits into from
Jul 30, 2015
Merged

[Embed API] Python Wrapper #62

merged 4 commits into from
Jul 30, 2015

Conversation

sup
Copy link
Contributor

@sup sup commented Jun 26, 2015

Mostly done, waiting for review

@sup sup closed this Jun 26, 2015
@sup
Copy link
Contributor Author

sup commented Jun 26, 2015

haha actually not done yet - was leaving a pr open for comments but it seems like i should wait till i have finished everything first

@talwai
Copy link

talwai commented Jun 26, 2015

Cool thanks, adding a test for the new functionality here would also be much appreciated

@sup sup reopened this Jul 15, 2015
@yannmh
Copy link
Member

yannmh commented Jul 15, 2015

@charleslai is it ready for review ?

@sup
Copy link
Contributor Author

sup commented Jul 15, 2015

@yannmh I think I'm good! let me know if anything needs to be fixed

@yannmh
Copy link
Member

yannmh commented Jul 15, 2015

Thanks a lot @charleslai !

Could you clean/concatenate your git commit history to have something short and clear ? Also could you remove the merge master commits ? it's making the PR heavier, and the history confusing (prefer a rebase instead) :)

@yannmh
Copy link
Member

yannmh commented Jul 15, 2015

Do we plan to bring this new feature to dogshell (aka dog command) ?

@sup
Copy link
Contributor Author

sup commented Jul 15, 2015

not at the moment I think

@sup
Copy link
Contributor Author

sup commented Jul 15, 2015

cleaned git history - add api wrapper and tests

@yannmh
Copy link
Member

yannmh commented Jul 16, 2015

Great, I'll review your PR soon. Could you also update the documentation to reflect your changes please ?

@sup
Copy link
Contributor Author

sup commented Jul 16, 2015

Documentation is updated


:returns: JSON response from HTTP API request
"""
return super(Embed, cls).get_all()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason why you redefine the method when no modification is made ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope - removing now

@yannmh
Copy link
Member

yannmh commented Jul 23, 2015

Great work @charleslai ! Thanks these new features.

It's ready to be merged 🚢.I just made a few nitpicky comments, if you want to adress them.

@sup
Copy link
Contributor Author

sup commented Jul 28, 2015

I think I addressed all of the comments - I didn't change the Action.. class because there is a method there called _trigger_class_action that I can use and this combined with _trigger_action seems to cover most use cases but let me know if I should alter _trigger_class_action

(_trigger_action can't really be changed because it doesn't use the class base url and instead goes for name + id and id + name wouldn't make too much sense)

@yannmh
Copy link
Member

yannmh commented Jul 29, 2015

Great work, thanks @charleslai.

Let's merge it and release a new package 🚀.

yannmh added a commit that referenced this pull request Jul 30, 2015
@yannmh yannmh merged commit 1c41915 into master Jul 30, 2015
@yannmh yannmh deleted the charles/embed_api_py branch July 30, 2015 13:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants