On Wed, 14 Apr 2010, Damyan Ivanov wrote: > Disclaimer: The following is my own opinion and I am not the only who > can apply the patches.
The patches I sent are mostly intended as ideas and attempts to improve the code. > -=| Cristian Ionescu-Idbohrn, Wed, Apr 14, 2010 at 12:39:21AM +0200 |=- > > On Tue, 13 Apr 2010, Santi Béjar wrote: > > > Send it in-line so we can easily comment your changes. > > > > Sure, I could do that and then find out others prefer attachments, because > > their mail servers/clients mangle the contents (wrap lines and damage > > whitespace). What do the maintainers prefer? > > Ideally as an attachment, using 'inline' content-disposition. I'll than have to hack my mail client, or use another ;) > Having the patches extracted using 'git format-patch' is preferred as > this would include the commit messages and authorship. Right. My git fu is limited. > A note about comments and verification of function arguments. These > are kept at minimum because some of the scripts are used in each ACPI > event and need to be as quick as possible. Yes. Performance is important. I do understand that. > Maybe we can ship the scripts without comments in the binary package, > as is done with the default file. That would make hacking of the > installed scripts harder though :/ Right. Maybe in a later phase. Ironing out bugs and improving performance at this stage is more important, IMO. > > Another observation is the 'notify' function seems to require > > 3 arguments, but never validates that. Never rely user input is > > correct. > > There is no user input. The only "user" are the other scripts in the > package. Is some of them not supplying three arguments? Of course, > validation can still be made, but I think performance will suffer. And > it works now, doesn't it? Seems to. I don't remember all the details, but I think I found (and commented on) questionable use of positional parameters $4 and $3. > > > - if [ "x$ENABLE_OSD" = "xno" ]; then > > > - return > > > - fi > > > + [ "$ENABLE_OSD" != no ] || return > > > > > > I don't like so many negative logic, > > I am used to that from makefiles, where it is inconvenient to split > commands on multiple lines, and commands are required to return true. > This is shell, though, so the readability of a multi-line if is > something I value more. That's ok, although parsing multiline conditions affects performance. And then again, there are a lot of things that must be considered. Finding the best balance isn't trivial. > > Why? All modern shells do the right thing without that ancient 'x'. > > That's bloat. And why do you need to quote something (non-null) that is > > not going to expand to anything different than it already is? In other > > words, isn't this readable and less bloaty? > > It is. Trimming your nails also helps you lose weight :) Fair comparison :) > > > - commands \ > > > - | su "$user" -c "$GOSDC -s --dbus" > > > + commands | > > > + su "$user" -c "$GOSDC -s --dbus" > > > > > > Why did you change the continuation sequence? > > > > Because shell is an iterpreted language. > > Because parsing shell commands in real time is expensive. This: > > > > foo \ > > | bar > > > > is subtilely more expensive than this: > > > > foo | > > bar > > I'd like to see some numbers backing this claim. I'll see if I find the time to do that at some point. > I find the former a bit more readable, but otherwise consider it a mater > of taste. Fair enough. > Personaly, I am much happier applying patches if they address problems > somebody is experiencing. Is the code so "bloated" and "ugly" as to > prevent contribution? No. But it's easier to work on non-bloated and less ugly code :) I recall I pointed out two bugs and provided patch code already. Cheers, -- Cristian _______________________________________________ Debian-eeepc-devel mailing list [email protected] http://lists.alioth.debian.org/mailman/listinfo/debian-eeepc-devel
