Re: [Gnewsense-dev] Debderive

[Sam Geeraerts]

Stayvoid wrote:
> Hello,
>
> Please check this patch. There is a pastebin copy. [1] I haven't
> checked it in a shell, but the syntax looks right.

I expect patches to have gone through some basic testing. It saves time 
for both of us. I let you take responsibility for your own code. So if 
there's a (trivial) bug in the patch I'd have to get back to you on it 
and ask you for a better patch. For you it would take longer to get your 
patch merged and start with something new. Each of the modified 
statements can be easily checked in a python shell or grouped together 
in a quick test script.

(snip)

It looks like this patch changes the following:
1) make error messages compatible with Python 3;
2) don't end error messages with a period;
3) quote paths;
4) replace commas with colons in error messages;
5) show an error message instead of raising an exception;
6) Check if configuration file exists.

Could you provide separate patches (and rationale) for each fix? It 
makes patches cleaner (if a patch introduces a bug, it's much easier to 
see what's wrong if it fixes only one issue). It makes commit logs 
cleaner (not "Fixes a bunch of stuff" or an enumeration of possibly 
unrelated fixes). It also makes discussion easier, allowing comments on 
a single change.

> I tried to follow this guide. [2, 3]

It's a shame the GNU coding standards don't give a rationale for not 
ending error messages with a period. I had a quick look at other 
projects and style guides. As usual, there's no concensus on the matter. 
The PostgreSQL style guide [a] says "avoiding punctuation makes it 
easier for client applications to embed the message into a variety of 
grammatical contexts". I'm ok with following GNU standards on this.

> Maybe we should make MAIN_CONF_FILE_NAME global to have a chance to
> call it in checks. What do you think?
> (MAIN_CONF_FILE_NAME = 'debderiver.yaml')

You could check if the file exists in the parse method. It's actually 
better to take opening the file out of the current yaml.load statement, 
put it into a separate try-block above it and give yaml.load the file 
object.

> [1] http://pastebin.com/raw.php?i=rN9Rj8uN
> [2] http://www.gnu.org/prep/standards/standards.html#Errors
> [3] http://www.gnu.org/prep/standards/standards.html#Quote-Characters

[a] http://www.postgresql.org/docs/current/static/error-style-guide.html