Thanks for your review!
On Sat, Nov 17, 2012 at 12:52 AM, Jakub Wilk <jwilk@debian.org> wrote:
> License in debian/copyright is not the same as in LICENSE.
Fixed upstream (I believe) — is that what you meant:
http://bazaar.launchpad.net/~mitya57/python-markups/trunk/revision/87/LICENSE ?
> Shouldn't python3-pygments be in Recommends?
python3-markdown and python3-docutils already recommend it, I don't
see why should it be recommended explicitly
> I'd use "filter" rather than "findstring" in debian/rules.
Fixed.
> Apart from these, the packaging looks good. I have some comments to the
> upstream part though:
>
>> CONFIGURATION_DIR = os.path.expanduser('~/.config/')
>
> Could you make it support XDG Base Directory Specification[0] instead of
> hardcoding ~/.config? Thanks.
>
>> try:
>> import markdown
>> except:
>> return False
>
>
> Make it s/except:/except ImportError:/. [1] (There are more instances of the
> same buglet in other files.)
>
>> orig_stylesheet = self.publish_parts(text)['stylesheet']
>> # Cut off <style> and </style> tags
>> return orig_stylesheet[25:-10] + get_pygments_stylesheet('.code')
>
> Eww, magic numbers. Wouldn't it break horribly if the user disabled
> embed_stylesheet in their docutils.conf?
Fixed in upstream branch.
>> start_position = head.find('<script ')
>> end_position = head.rfind('</script>')
>> if start_position >= 0 and end_position >= 0:
>> mjurl = head[start_position:end_position+9]+'\n'
>> return mjurl.replace(MATHJAX_WEB_URL, get_mathjax_url(webenv))
>> return ''
>
> Eww again. How about using regular expressions instead? :)
9 here is just len('</script>') — this will never change :)
I'll make a new upstream release soon and then update the packaging.
--
Dmitry Shachnev