[gobolinux-users] Gobo wants you! (to comment its scripts)

[Daniele Maccari]

Jonas Karlsson wrote:
> Note, when I give references to the code, I refer to line numbers in the
> current svn, that is, with the patch applied.
>
> On 11/04/2008, Michael Homer <michael at gobolinux.org> wrote:
>   
>> On Fri, Apr 11, 2008 at 8:51 PM, Daniele Maccari <gobo.users at gmail.com> wrote:
>>  > Mmm, well, I'm doing to change anything anybody will find erroneous/not
>>  > clear, so keep suggestion coming.
>>
>> It's funny you should say that. After some discussions on IRC:
>>  - Some of the (inline) comments are a bit unnecessary ("Print out
>>  notes for this script, if any.", "Quiet suppresses output"). I think
>>  they're remnants of when you were commenting for your own
>>  understanding, so they could be removed.
>>     
> I too agree that the comments are a bit verbose. The reason, not the
> code, should be explained by comments, unless one can't see what
> the specific code does without explanation. Michael gives a good
> example below with "Print out examples".
>   
See my reply to michal about "section comments".
>   
>>  - For if/then, the comment should be either on the same line as the
>>  then, or on the subsequent lines, not in between the if and then
>>  (there are two of these)
>>     
>
> My opinion is that one liners should be on the same line as the
> then/else/fi while multiple lines should come after (except for "fi" where
> no multiple lines should be used).
>
>   
I usually put multiline if-else explanations just before them, how would 
you that instead?
The case pointed at by Michael was something like:

if [...]
# comments
# comments
then

is it what you mean?
>>  - Jonas doesn't like you correcting his grammar. I think you should do
>>  it anyway, because I like consistency.
>>     
> I don't mind my grammar being corrected, if it's wrong. My comment was
> that unnecessary changes to comments adds too much noice and adds
> no value. My example was (around line 860):
>
>                if [ "$opt" = "--${optionsLongList[j]}" ]
>                then
> -                  # we found a valid (known) option
> +                  # We've found a valid (known) option
>
>   
As Michael said, I mainly changed it for consistency, since every other 
line of comment starts
capitalized. The tense instead has been a just impulsive thing. I've 
just noticed I also forgot the period at the end :D
> English is not my native language, but I thought that the original
> sentence was correct and the change provided no value. A more valid
> example would be the changes just some lines below:
>
>                      if echo "$option" | grep -q = -
>                      then
> -                        # long option value assignment with =
> +                        # Long option value assignment with =.
>
> where the changes add no value at all imo.
>   
As before, just consistency.
>>  - The blank "#<space>" lines in function comments (which are otherwise
>>  great) take up a bit much room. You can only fit so many lines on a
>>  screen, so blank ones lose you some real estate.
>>     
> Also some newlines added besides the comments coincidence with
> indentation level changes, where the indentation itself allows the code to
> be easy read (imo).
>   
Couldn't get it, what do you mean?
>   
>>  - It was also resolved to general agreement that I should say these
>>  things *before* I commit patches. I suppose that was a valid point,
>>  all things considered.
>>
>>     
> Yes. :)
> Besides the points made above there are also some errors made in the
> comments and changes that, to me, doesn't clarify the code. I don't have
> time to review the code thoroughly, but at a glance I can give one
> example of both.
>
> Around line 850:
>
> -            # Go through the list of available options
> +            # Loop through options.
>
> "loop" adds value, while "available" is dropped, so either comment is
> bad. "Loop through <available|valid> options" would be better.
>
>   
Good point, my fault here. Valid seems better to me.
> Around line 970:
>
> -         fi
> -      else # dash found
> +         fi # dash found
> +      else
> +         # The parameter isn't an option.
>
> the "dash found" should be at the "else", not the "fi".
>
>   
Mmm, that's one thing I wondered about: imo, reading "#dash found" on 
the same line as else, could be interpreted as a comment about what the 
else block does, while this is not the case, iirc. Normally such lines 
should go just after a block's closing bracket, but not having it here, 
I thought placing it after the if would have been more comprehensible, 
to some extent.
I could've surely been wrong, of course :D

P.S.: Thanks for the second message showing a bit of pity, I know you, 
but sometimes a simple thanks can mean a lot ;)