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

Add Location header to Publish response #463

Merged
merged 3 commits into from Sep 6, 2015
Merged

Add Location header to Publish response #463

merged 3 commits into from Sep 6, 2015

Conversation

valpackett
Copy link
Contributor

https://indiewebcamp.com/syndication-brainstorming

Should probably also switch to returning 201 Created... I hope existing clients check for 2xx and not exactly 200?

@snarfed
Copy link
Owner

snarfed commented Sep 6, 2015

thanks! would fix #462. looking now.

@snarfed
Copy link
Owner

snarfed commented Sep 6, 2015

looks great! any interest in testing it out? specifically we'd usually want to update a unit test in test_publish.py before merging a change like this.

@valpackett
Copy link
Contributor Author

Sure! Got the tests working (one Flickr-related test failed with ValueError: Only unicode objects are escapable. Got None of type <type 'NoneType'> in oauthlib/oauth1/rfc5849/utils.py, line 56, in escape).

It's really weird that the README mentions symlinking ../../../src/... – there's no way pip under virtualenv would install something to ../../../ – that makes no sense! The right commands were:

ln -s $(pwd)/local/src/gdata/src/gdata local/lib/python2.7/site-packages/gdata
ln -s $(pwd)/local/src/gdata/src/atom local/lib/python2.7/site-packages/atom

@kylewm
Copy link
Contributor

kylewm commented Sep 6, 2015

symlinking to the absolute path definitely works too, but relative paths worked for me (I just recreated my virtualenv this morning)

(local)kmahan@lemur:~/projects/bridgy/local/lib/python2.7/site-packages$ ls -l
lrwxrwxrwx  1 kmahan kmahan    27 Sep  6 08:09 atom -> ../../../src/gdata/src/atom
lrwxrwxrwx  1 kmahan kmahan    28 Sep  6 08:09 gdata -> ../../../src/gdata/src/gdata

@valpackett
Copy link
Contributor Author

Ah, relative to local/lib/python2.7/site-packages. But the README does not mention cding into that directory!! I'm still in the bridgy repo root when I'm following the README.

@snarfed
Copy link
Owner

snarfed commented Sep 6, 2015

yeah, symlinking is funny like that. it actually uses the literal target string you give it, so it doesn't resolve relative paths like .. at symlink creation time but at each access time, from the symlink's own location.

@valpackett
Copy link
Contributor Author

Yeah. Right. I get confused by this every time :-(

Anyway, added the test, it works!

@snarfed
Copy link
Owner

snarfed commented Sep 6, 2015

woo, thanks for the test, and congrats on landing the patch!

(fixes #462)

snarfed added a commit that referenced this pull request Sep 6, 2015
Add Location header to Publish response
@snarfed snarfed merged commit 69bbb9b into snarfed:master Sep 6, 2015
@aaronpk
Copy link
Contributor

aaronpk commented Sep 6, 2015

It looks like this doesn't change the HTTP code to 201.

When I was testing returning a Location header in a PHP framework, something was changing the status code to 302 when there is a Location header if I didn't explicitly set the status code to 201. I think this is because HTTP only allows a location header in a 201 and 30x response. I'm not sure if this Python environment will do something similar, but it would be good to return 201 regardless.

http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html#sec10.2.2

@valpackett
Copy link
Contributor Author

Looks like my initial comment was ignored:

Should probably also switch to returning 201 Created... I hope existing clients check for 2xx and not exactly 200?

(The reason I'm concerned about clients is because Micropublish used to expect exactly 200 from the token endpoint, and mine returns 201. Fixed it, of course.)

@snarfed
Copy link
Owner

snarfed commented Sep 6, 2015

sorry for forgetting the 201 part @aaronpk. and @myfreeweb fair concern. i don't know for sure, but I'd guess most clients handle 2xx. any interest in making the status code change too?

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.

None yet

4 participants