loggerhead

Merge lp:~mwhudson/loggerhead/relative-links into lp:loggerhead

  1. relative-links
  2. Merge into trunk-rich
Proposed by Michael Hudson-Doyle
Status: Rejected
Rejected by: Matt Nordhoff
Proposed branch: lp:~mwhudson/loggerhead/relative-links
Merge into: lp:loggerhead
Diff against target: 30 lines (+8/-5)
1 file modified
loggerhead/apps/branch.py (+8/-5)
To merge this branch: bzr merge lp:~mwhudson/loggerhead/relative-links
Reviewer Review Type Date Requested Status
Matt Nordhoff Disapprove
Review via email: mp+15298@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Michael Hudson-Doyle (mwhudson) wrote :

This branch generates relative links apart from in redirects.

Revision history for this message
Matt Nordhoff (mnordhoff) wrote :

Poking around with this patch, it works, AFAICT.

I don't know enough about Loggerhead/Paste's URL handling to review it, though.

However, there is one problem, the feeds:

* <id> needs to be changed back to use absolute URLs.

* The Feed Validator also recommends using absolute URLs for <link rel="self" href="..." /> for maximum interoperability.

* The rest of the feeds can stick with relative URLs.

Once that's fixed, if you're sure nothing else needs to be changed, I have no objections to landing this. But as I said, I don't think I have the expertise to know if I should have any objections. :P

review: Needs Fixing
Revision history for this message
Matt Nordhoff (mnordhoff) wrote :

> * The Feed Validator also recommends using absolute URLs for <link rel="self"
> href="..." /> for maximum interoperability.

NB: It doesn't mind relative URLs in the other <link> tags.

Revision history for this message
Matt Nordhoff (mnordhoff) wrote :

Superseded by lp:~mnordhoff/loggerhead/relative-links.

review: Disapprove

Unmerged revisions

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'loggerhead/apps/branch.py'
2--- loggerhead/apps/branch.py 2009-10-17 06:35:33 +0000
3+++ loggerhead/apps/branch.py 2009-11-27 00:35:21 +0000
4@@ -88,10 +88,11 @@
5 if v is not None:
6 qs.append('%s=%s' % (k, urllib.quote(v)))
7 qs = '&'.join(qs)
8- return request.construct_url(
9- self._environ, script_name=self._url_base,
10- path_info=unicode('/'.join(args)).encode('utf-8'),
11- querystring=qs)
12+ path_info = urllib.quote(
13+ unicode('/'.join(args)).encode('utf-8'), safe='/~:')
14+ if qs:
15+ path_info += '?' + qs
16+ return self._url_base + path_info
17
18 def context_url(self, *args, **kw):
19 kw = util.get_context(**kw)
20@@ -152,7 +153,9 @@
21 path = request.path_info_pop(environ)
22 if not path:
23 raise httpexceptions.HTTPMovedPermanently(
24- self._url_base + '/changes')
25+ request.construct_url(
26+ self._environ, script_name=self._url_base,
27+ path_info='/changes'))
28 if path == 'static':
29 return static_app(environ, start_response)
30 cls = self.controllers_dict.get(path)

Subscribers

People subscribed via source and target branches