yellow team mailing list archive

[Francesco Banconi <frankban@xxxxxxxxx>]

> I was struck that file_append really expects the line to be added to end with
> a \n.  I suppose we could enforce that one way or another...or not.  I'm OK
> with leaving as is, or you could add a \n if the line does not already end
> with one (and do this before you check if the line is in the content).  On a
> somewhat related note, it might be better to check if the line to be added is
> in the output of readlines(), not read(); that would verify that the line, not
> a line that ends with the line, is in the file. For example, right now a file
> of "abc\ndef\n" would incorrectly show that the "ef\n" line was in the file.

I agree with your suggestions. I've changed the function and removed those
ambiguities. I've added tests for line not ending with '\n' and for line
fragments.

> In file_prepend, I am tempted to suggest that this snippet should be changed:
>             if line in lines:
>                 lines.remove(line)
> This iterates through the lines twice, which is mildly annoying to me but
> almost certainly not a practical concern for us with these use cases.  That
> said, I'd be tempted to change that to the following:
>             try:
>                 lines.remove(line)
>             except ValueError:
>                 pass
> You could also verify that the line to be added in this function ended with a
> \n if you wanted to.

Done.

> I'm a bit concerned about what you are doing here in get_value_from_line:
>     return line.split('=')[1].strip('"\' ')
> would maybe json be a safer approach?  No, I tried it, and json.loads('"hi"')
> is fine but json.loads("'hi'") is not. :-/  I dunno.  When do we use this?
> The logic seems a bit idiosyncratic.

I don't remember the origin of that function. Since it is not used in the
charms or in setuplxc/lpsetup, I am going to remove it.

> The grep command also seems a bit idiosyncratic.  Why is it reasonable to
> always strip the result?  I also would be tempted to rename the funtion: grep
> does a lot more than that.  simply search_file?  Hm, I see that you just
> reordered this file, you didn't actually add grep....  Never mind then, I
> guess.  I should have commented on this sooner, but you shouldn't have to
> worry about it.  If you *do* want to change it, please leave the "grep" alias
> around for now so that our charms do not suddenly break.

Actually that helper is not used, and I remember it was before we changed the
way the buildslave is reconfigured. I think it's safe to rename it and change
the returned value.

> I don't understand why you removed the Serializer.  Was it defined twice?

Yes it was.

Thanks Gary.
-- 
https://code.launchpad.net/~frankban/python-shell-toolbox/helpers/+merge/96180
Your team Launchpad Yellow Squad is subscribed to branch lp:python-shell-toolbox.