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

Alt text - work for issue 316#319

Merged
jeremylow merged 5 commits intomasterfrom
alt-text_issue-316
May 17, 2016
Merged

Alt text - work for issue 316#319
jeremylow merged 5 commits intomasterfrom
alt-text_issue-316

Conversation

@bear
Copy link
Copy Markdown
Owner

@bear bear commented Apr 8, 2016

This change is Reviewable

@bear bear mentioned this pull request Apr 8, 2016
@jeremylow
Copy link
Copy Markdown
Collaborator

Do we want to make a character length check and an int check on the values of the kwargs? I think we'd also want to integrate posting metadata with the PostMedia*() functions, no?

Maybe as well via PostUpdate()? Since PostUpdate doesn't return the media ID directly (you'd have to go hunting for it in the Status object that's returned) it makes sense to allow passing the parameter so you don't need to do a bunch contortions to post a status, then get the media ID, then upload the alt text.

Also, weird that Twitter's using nested json here; that's the only one in the API right?

@bear
Copy link
Copy Markdown
Owner Author

bear commented Apr 8, 2016

it's the only one I found searching for alt_text - all of the other media related ones didn't have anything

I like the idea of bundling it with the other media calls to reduce the number of hops

@jeremylow
Copy link
Copy Markdown
Collaborator

Looks like requests.post is mangling the JSON payload because it's getting passed via the data= argument rather than the json= argument. Mind if I throw a commit at this?

@bear
Copy link
Copy Markdown
Owner Author

bear commented Apr 9, 2016

please adjust/alter this as needed - I created it because I was sitting with the editor open and the twitter dev page and could knock out the start really quickly.

jeremylow added 3 commits May 10, 2016 06:08
when posting json data, _RequestURL mangles the payload causing a 400 error from twitter. to fix: allow posting a json payload through _RequestURL by passing it off to requests.post via the json parameter rather than the data parameter.
@jeremylow
Copy link
Copy Markdown
Collaborator

So as this basically works, I'm OK sending this to master with the caveat that maybe we should keep the issue open, since we still don't have real documentation as to how this change is going to affect other endpoints (i.e., how this gets consumed across the API rather than created). What are your thoughts? I'd like this to be available since it's an accessibility improvement and anything is better than the current state of affairs, but it's also somewhat incomplete and not particularly well documented.

@bear
Copy link
Copy Markdown
Owner Author

bear commented May 14, 2016

I say let's merge it and add a section to the docs about something like "Changes" or something - basically a roadmap of changes from what is old/stable to what is in master branch?

@jeremylow
Copy link
Copy Markdown
Collaborator

Sounds good. I guess that's like a more fleshed out change log? Something like the migration guide from 2x to 3x? http://python-twitter.readthedocs.io/en/latest/migration_v30.html

@bear
Copy link
Copy Markdown
Owner Author

bear commented May 16, 2016

yea, the change log seems to be more focused and specific to release/commits.

This would be more of a guided tour thru the new/changed features aimed at someone who is already using it

@bear
Copy link
Copy Markdown
Owner Author

bear commented May 16, 2016

Reviewed 3 of 3 files at r3, 2 of 2 files at r5.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@jeremylow jeremylow merged commit c5e37b1 into master May 17, 2016
@jeremylow
Copy link
Copy Markdown
Collaborator

Cool. How do you want to do versioning in that case? Looks like we're still calling master 3.0rc1 (which we should probably fix to be 3.0), so this could be included in a 3.1 release or rolled into 3.0 since that's not official yet.

@bear bear deleted the alt-text_issue-316 branch May 17, 2016 05:08
@bear
Copy link
Copy Markdown
Owner Author

bear commented May 17, 2016

yea, we should probably deploy 3.0 now -- it's stable enough and people have had time to react.

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.

2 participants

X Tutup