From mboxrd@z Thu Jan  1 00:00:00 1970
From: john at keeping.me.uk (John Keeping)
Date: Tue, 19 Jun 2018 09:34:57 +0100
Subject: [PATCH v2 14/15] md2html-add-asset-postfix-arg
In-Reply-To: <50162463-ab59-5200-5996-0f821b65ac26@warmcat.com>
References: <152928998685.10419.7869045561776063625.stgit@mail.warmcat.com>
 <152929070596.10419.8074350024589074648.stgit@mail.warmcat.com>
 <20180618192136.GQ1922@john.keeping.me.uk>
 <50162463-ab59-5200-5996-0f821b65ac26@warmcat.com>
Message-ID: <20180619083457.GW1922@john.keeping.me.uk>

On Tue, Jun 19, 2018 at 11:55:35AM +0800, Andy Green wrote:
> 
> 
> On 06/19/2018 03:21 AM, John Keeping wrote:
> > On Mon, Jun 18, 2018 at 10:58:26AM +0800, Andy Green wrote:
> 
> >>   class AssetMappingExtension(markdown.extensions.Extension):
> >>   
> >>       def __init__(self, **kwargs):
> >> -        self.config = {'asset_prefix': ['', 'prefix for relative asset URLs']}
> >> +        self.config = {'asset_prefix': ['', 'prefix for relative asset URLs'], 'asset_postfix': ['', 'postfix for relative asset URLs']}
> > 
> > For style it would be nice to align this under asset_prefix.
> 
> Just worried to upset the python indent monster... it seems to work 
> intended I guess it only cares at the start of statements.

Yeah, if you're inside braces of any kind you can pretty much do what
you want.

> >>           super(AssetMappingExtension, self).__init__(**kwargs)
> >>   
> >>       def extendMarkdown(self, md, md_globals):
> >>           asset_prefix = self.getConfig('asset_prefix')
> >>           if not asset_prefix:
> >>               return
> >> +        asset_postfix = self.getConfig('asset_postfix')
> >> +        if not asset_postfix:
> >> +            return
> > 
> > Is this right?  Should we allow one of these to be empty and still
> > process the other one?  In other words, shouldn't the bail out condition
> > be:
> > 
> >      if not (asset_prefix or asset_postfix):
> >          return
> 
> Yeah... the original code that generated the args always generated a 
> postfix, so there was "no problem".
> 
> However the improved code snips ?h=defaultbranch and can be an empty 
> string.  So I changed this as you suggested.
> 
> > I don't think any change to AssetMappingProcessor is required because
> > urljoin already does the right thing when handed the empty string and
> > the config assure that if no value is specified then that is what we
> > get.
> 
> I am not certain of your meaning here... you mean change from what this 
> patch already does for the reason above?  It must change 
> AssetMappingProcessor generally to do what it's trying to do.

I mean that changing the code here will allow empty strings to be passed
to AssetMappingProcessor.run() which previously was blocked by these
conditions.  But no change is required there because it already does the
right thing when handed an empty string as one of prefix/postfix.