cms/converters.py - Issue 29933596: Issue 5333 - Allow cms to generate relative pages

Delta Between Two Patch Sets: cms/converters.py

Issue 29933596: Issue 5333 - Allow cms to generate relative pages (Closed) Base URL: https://hg.adblockplus.org/cms/

Left Patch Set: Allow cms to generate relative pages Created Nov. 2, 2018, 2:59 a.m.

Right Patch Set: Add documentation, remove extraneous global Created Nov. 9, 2018, 8:16 p.m.

Left:
Right:

Use n/p to move between diff chunks; N/P to move between comments.

Jump to:

Left: Side by side diff | Download
Right: Side by side diff | Download

LEFT	RIGHT
1 # This file is part of the Adblock Plus web scripts,	1 # This file is part of the Adblock Plus web scripts,

2 # Copyright (C) 2006-present eyeo GmbH	2 # Copyright (C) 2006-present eyeo GmbH

3 #	3 #

4 # Adblock Plus is free software: you can redistribute it and/or modify	4 # Adblock Plus is free software: you can redistribute it and/or modify

5 # it under the terms of the GNU General Public License version 3 as	5 # it under the terms of the GNU General Public License version 3 as

6 # published by the Free Software Foundation.	6 # published by the Free Software Foundation.

7 #	7 #

8 # Adblock Plus is distributed in the hope that it will be useful,	8 # Adblock Plus is distributed in the hope that it will be useful,

9 # but WITHOUT ANY WARRANTY; without even the implied warranty of	9 # but WITHOUT ANY WARRANTY; without even the implied warranty of

10 # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the	10 # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the

11 # GNU General Public License for more details.	11 # GNU General Public License for more details.

12 #	12 #

13 # You should have received a copy of the GNU General Public License	13 # You should have received a copy of the GNU General Public License

14 # along with Adblock Plus. If not, see <http://www.gnu.org/licenses/>.	14 # along with Adblock Plus. If not, see <http://www.gnu.org/licenses/>.

15	15

16 from __future__ import unicode_literals	16 from __future__ import unicode_literals

17	17

18 import os	18 import os

19 import HTMLParser	19 import HTMLParser

20 import re	20 import re

21 import urlparse	21 import urlparse

	22 from posixpath import relpath

22	23

23 import jinja2	24 import jinja2

24 import markdown	25 import markdown

25	26

26 from cms import utils	27 from cms import utils

27	28

28 # Monkey-patch Markdown's isBlockLevel function to ensure that no paragraphs	29 # Monkey-patch Markdown's isBlockLevel function to ensure that no paragraphs

29 # are inserted into the <head> tag	30 # are inserted into the <head> tag

30 orig_isBlockLevel = markdown.util.isBlockLevel	31 orig_isBlockLevel = markdown.util.isBlockLevel

31	32

(...skipping 259 matching lines...) Expand 10 before \| Expand all \| Expand 10 after Loading...
291 r'{{(?:(?!}}).)*}}' # Nested translation	292 r'{{(?:(?!}}).)*}}' # Nested translation

292 r')*?)'	293 r')*?)'

293 r')?'	294 r')?'

294 r'}}',	295 r'}}',

295 lookup_string,	296 lookup_string,

296 text,	297 text,

297 flags=re.S,	298 flags=re.S,

298 )	299 )

299	300

300 def process_links(self, text):	301 def process_links(self, text):

301 def make_relative(source, target):	302 def make_relative(base_url, target):
Vasily Kuznetsov 2018/11/02 10:45:35 It would probably be good to rename `source` to `b It would probably be good to rename `source` to `base_url` to use the terminology from the relevant RFC. The code would be easier to follow. rhowell 2018/11/06 03:34:41 Done. Show quoted text On 2018/11/02 10:45:35, Vasily Kuznetsov wrote: > It would probably be good to rename `source` to `base_url` to use the > terminology from the relevant RFC. The code would be easier to follow. Done.
302 if not target.startswith('/'):	303 if not target.startswith('/'):

303 # Links to an external resource	304 # Links to an external resource

304 return target	305 return target

305 if source == target:	306 return relpath(target, base_url.rsplit('/', 1)[0])

306 target = re.split('/', target)
Vasily Kuznetsov 2018/11/02 10:45:35 If this better than target.split('/')? If this better than target.split('/')? rhowell 2018/11/06 03:34:41 Done. Show quoted text On 2018/11/02 10:45:35, Vasily Kuznetsov wrote: > If this better than target.split('/')? Done.
307 return target[-1]

308 return os.path.relpath(target, source)
Vasily Kuznetsov 2018/11/02 10:45:35 Here we need to take the part of `source` (or `bas Here we need to take the part of `source` (or `base_url`) up to the last slash. Something like `source.rsplit('/', 1)[0]`. rhowell 2018/11/06 03:34:41 Or, maybe it's cleaner to just pass the base path Show quoted text On 2018/11/02 10:45:35, Vasily Kuznetsov wrote: > Here we need to take the part of `source` (or `base_url`) up to the last slash. > Something like `source.rsplit('/', 1)[0]`. Or, maybe it's cleaner to just pass the base path into this function in the first place, and not include the current page when we first make the current_page variable on line 325-326? Vasily Kuznetsov 2018/11/06 12:45:16 This is not quite right. First, "base URL" has spe Show quoted text On 2018/11/06 03:34:41, rhowell wrote: > On 2018/11/02 10:45:35, Vasily Kuznetsov wrote: > > Here we need to take the part of `source` (or `base_url`) up to the last > slash. > > Something like `source.rsplit('/', 1)[0]`. > > Or, maybe it's cleaner to just pass the base path into this function in the > first place, and not include the current page when we first make the > current_page variable on line 325-326? This is not quite right. First, "base URL" has specific meaning in the RFC and it is the URL of the original page (unless the page overrides it, but we presume it doesn't). Second, in your implementation (that means "containing folder" by "base URL") the base URL is equal to "/" + locale, which is not the right containing folder for pages that are in a folder (for example is the source is "pages/foo/bar", it will be rendered to "/en/foo/bar" and the containing folder would be "/en/foo"). rhowell 2018/11/07 16:16:58 Ok, I think I get it now. I've tried a different w Show quoted text On 2018/11/06 12:45:16, Vasily Kuznetsov wrote: > On 2018/11/06 03:34:41, rhowell wrote: > > On 2018/11/02 10:45:35, Vasily Kuznetsov wrote: > > > Here we need to take the part of `source` (or `base_url`) up to the last > > slash. > > > Something like `source.rsplit('/', 1)[0]`. > > > > Or, maybe it's cleaner to just pass the base path into this function in the > > first place, and not include the current page when we first make the > > current_page variable on line 325-326? > > This is not quite right. First, "base URL" has specific meaning in the RFC and > it is the URL of the original page (unless the page overrides it, but we presume > it doesn't). Second, in your implementation (that means "containing folder" by > "base URL") the base URL is equal to "/" + locale, which is not the right > containing folder for pages that are in a folder (for example is the source is > "pages/foo/bar", it will be rendered to "/en/foo/bar" and the containing folder > would be "/en/foo"). Ok, I think I get it now. I've tried a different way to get the current page (using the pagedata info, and grabbing whatever comes after `/pages/`), and then splitting it as you suggested when passing it to `relpath()`. Is this more what you had in mind?
309	307

310 def process_link(match):	308 def process_link(match):

311 pre, attr, url, post = match.groups()	309 pre, attr, url, post = match.groups()

312 url = jinja2.Markup(url).unescape()	310 url = jinja2.Markup(url).unescape()

313	311

314 locale, new_url = (	312 locale, new_url = (

315 self._params['source']	313 self._params['source']

316 .resolve_link(url, self._params['locale']))	314 .resolve_link(url, self._params['locale']))

317	315

318 if new_url is not None:	316 if new_url is not None:

(...skipping 120 matching lines...) Expand 10 before \| Expand all \| Expand 10 after Loading...
439 'linkify': self.linkify,	437 'linkify': self.linkify,

440 'toclist': self.toclist,	438 'toclist': self.toclist,

441 }	439 }

442	440

443 globals = {	441 globals = {

444 'get_string': self.get_string,	442 'get_string': self.get_string,

445 'has_string': self.has_string,	443 'has_string': self.has_string,

446 'get_page_content': self.get_page_content,	444 'get_page_content': self.get_page_content,

447 'get_pages_metadata': self.get_pages_metadata,	445 'get_pages_metadata': self.get_pages_metadata,

448 'get_canonical_url': self.get_canonical_url,	446 'get_canonical_url': self.get_canonical_url,

449 'relative': self._params['relative'],

450 }	447 }

451	448

452 for dirname, dictionary in [('filters', filters),	449 for dirname, dictionary in [('filters', filters),

453 ('globals', globals)]:	450 ('globals', globals)]:

454 for filename in self._params['source'].list_files(dirname):	451 for filename in self._params['source'].list_files(dirname):

455 root, ext = os.path.splitext(filename)	452 root, ext = os.path.splitext(filename)

456 if ext.lower() != '.py':	453 if ext.lower() != '.py':

457 continue	454 continue

458	455

459 path = os.path.join(dirname, filename)	456 path = os.path.join(dirname, filename)

(...skipping 132 matching lines...) Expand 10 before \| Expand all \| Expand 10 after Loading...
592 stack[-1]['subitems'].append(item)	589 stack[-1]['subitems'].append(item)

593 stack.append(item)	590 stack.append(item)

594 return structured	591 return structured

595	592

596	593

597 converters = {	594 converters = {

598 'html': RawConverter,	595 'html': RawConverter,

599 'md': MarkdownConverter,	596 'md': MarkdownConverter,

600 'tmpl': TemplateConverter,	597 'tmpl': TemplateConverter,

601 }	598 }

LEFT	RIGHT