[PATCH v2 14/15] md2html-add-asset-postfix-arg

[Andy Green]

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.

>>           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.

>> -if len(sys.argv) > 2:
>> -    extensions.append(AssetMappingExtension(asset_prefix=sys.argv[2]))
>> +if len(sys.argv) > 3:
>> +    extensions.append(AssetMappingExtension(asset_prefix=sys.argv[2],asset_postfix=sys.argv[3]))
> 
> Can we allow specifying only the prefix here?  Something like:
> 
>      if len(sys.argv) > 2:
>          args = {'asset_prefix': sys.argv[2]}
>          if len(sys.argv) > 3:
>              args['asset_postfix'] = sys.argv[3]
>          extensions.append(AssetMappingExtension(**args))

OK.

-Andy