python/samba/ms_forest_updates_markdown.py (was Re: [PATCH] remove "ExistingBackend"...)

Douglas Bagnall douglas.bagnall at catalyst.net.nz
Thu Nov 1 20:49:35 UTC 2018


hi Noel,

On 2/11/18 2:11 AM, Noel Power wrote:
> Hi Douglas
> 
> On 01/11/2018 10:05, Noel Power wrote:
>>
>> On 31/10/2018 20:57, Douglas Bagnall wrote:
>>> On 31/10/18 11:47 PM, Noel Power wrote:
>>>>   [PATCH 24/29] python/ms_forest_updates_markdown: avoid implicit
>>>>   global variable
>>>>
>>>>
>>>> -    read_ms_markdown(in_file, out_folder)
>>>> +    read_ms_markdown(in_file, out_folder, {})
>>>>
>>>> That looks wrong to me, read_ms_markdown looks like it takes a file or
>>>> dictionary, it looks like the code will prefer the file if both are
>>>> specified but read_ms_markdown should either pass a temp dict to
>>>> save_array routine or save_array should handle the None dict, it should
>>>> handle the None default gracefully
>>>>
>>> Right. It just needs the attached version then? I think you are right
>>> and it looks clearer.
>>
>> RB+
>>
> 
> looking at it again, I think we need to be a bit careful, it seems there
> is a possibility that None could get passed to save_array
> 
>>                     if filename and out_folder is not None:
>>                         save_ldif(filename, result, out_folder)
>>                     else:
>>                         save_array(guid, result, out_dict)
> 
> we pass out_folder and dict to read_ms_markdown but filename is gleaned
> from the input html

'filename' depends on 'guid' and essentially nothing else, so it should
be OK. They co-exist or neither exists.

It gets worse the more you look, of course. If 'filename' is not set, it
is not a falsey value like None, it is simply undefined. So the error
happens at

                     if filename and out_folder is not None:

with a NameError. At least it's failing early.

> so... for the above it is possible that save_array could be called with
> None, maybe this should be an error ?
> 
> Or we could always create a temp out_dict at the top of
> read_ms_markdown. I don't think it makes sense to default out_dict in
> save_array (and passing None to save_array *should* be an error and fail
> imo) Thoughts ?

I mostly think we should stop looking. This particular read_ms_markdown
function is never called with the out_folder option outside of the "if
__name__ == '__main__'" block, and it only ever has to parse one file
(source4/setup/adprep/WindowsServerDocs/Forest-Wide-Updates.md). We can
let it be if it seems to work.

Douglas



More information about the samba-technical mailing list