Skip to content
This repository was archived by the owner on Aug 7, 2024. It is now read-only.

Adding language support to GetStreamFilter in api.py. Also added the example for that. - #401

Merged
jeremylow merged 6 commits into
bear:developfrom
chaitanya0411:master
Nov 3, 2016
Merged

Adding language support to GetStreamFilter in api.py. Also added the example for that.#401
jeremylow merged 6 commits into
bear:developfrom
chaitanya0411:master

Conversation

@chaitanya0411

@chaitanya0411 chaitanya0411 commented Nov 1, 2016

Copy link
Copy Markdown
Contributor

This is my first contribution to somebody else's repository, so please pardon me if I have made some blunder :)


This change is Reviewable

…Also added the corresponding example for use.
…nd changed a comment in the newly added streaming example
@codecov-io

codecov-io commented Nov 1, 2016

Copy link
Copy Markdown

Current coverage is 74.20% (diff: 0.00%)

Merging #401 into master will decrease coverage by 0.07%

@@             master       #401   diff @@
==========================================
  Files             8          8          
  Lines          1971       1973     +2   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
  Hits           1464       1464          
- Misses          507        509     +2   
  Partials          0          0          

Powered by Codecov. Last update d3eb170...2b25d70

@jeremylow

Copy link
Copy Markdown
Collaborator

Looks pretty good! A couple things I'd like to see:

  • Could you update doc/changelog.rst to note the change?
  • Since it's only a few lines different in track_users.py, it may make sense to incorporate the changes on lines 20, 57, & 71 into the existing file. That way if we have to update the examples, we just make changes in one place.

Let me know what you think on # 2.

@jeremylow

Copy link
Copy Markdown
Collaborator

(Don't worry about the code coverage thing - that function isn't tested yet anyway.)

@jeremylow

Copy link
Copy Markdown
Collaborator

Looks great! Thanks for your help! @bear you good with this?

@bear

bear commented Nov 2, 2016

Copy link
Copy Markdown
Owner

Reviewed 2 of 3 files at r2.
Review status: 2 of 3 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


twitter/api.py, line 4455 at r2 (raw file):

                        track=None,
                        locations=None,
                        languages=None,

my only quibble is that the new parameter be added to the end because we are doing this in a point release.

if we add it into the middle of the parameter list then we break anyone who doesn't use named parameters in their calls.


Comments from Reviewable

@jeremylow

Copy link
Copy Markdown
Collaborator

Crap, I did the same thing with PostStatus in #394

@bear

bear commented Nov 2, 2016

Copy link
Copy Markdown
Owner

@jeremylow ugh, sorry for not catching that! the benefit of smaller sized PRs

@jeremylow

Copy link
Copy Markdown
Collaborator

Yep. I'll fix it on develop.

@jeremylow
jeremylow changed the base branch from master to develop November 2, 2016 23:54
@jeremylow

Copy link
Copy Markdown
Collaborator

Speaking of develop, I just noticed this is going into the master branch, so I switched it to get integrated with the rest of the changes for 3.2.

@bear

bear commented Nov 2, 2016

Copy link
Copy Markdown
Owner

with that param change i'm +1 on merging - nicely done!

@jeremylow
jeremylow merged commit 6ca83cb into bear:develop Nov 3, 2016
@jeremylow

Copy link
Copy Markdown
Collaborator

Awesome. Merged! Thanks @chaitanya0411 and congrats on your first PR!

@chaitanya0411

Copy link
Copy Markdown
Contributor Author

Thanks guys ! It was a good experience !
I hope there are more PR to come from me...
Also, as mentioned at https://python-twitter.readthedocs.io/en/latest/getting_started.html ,
I would like to check a link to my example application that I built using python-twitter.
Please free to comment or link it as an example ! Thanks again :)

jeremylow added a commit that referenced this pull request Nov 23, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants