Subject: [bug#57598] [PATCH] doc: Update contribution guidelines on patches, etc.

From: Maxime Devos <[email-masked]>
Date: Fri, 13 Oct 2023 16:14:53 +0200

>> +Guix has tree main ways of modifying the source code of a package, that
>> +you as a packager may use.  These are patches, snippets and phases.
>                             ^
> “may use: patches, snippets, and build phases.”

OK, for the next version of the patch I won't write.

>> +Each one has its strengths and drawbacks.  To decide between the three,
> 
> s/decide between the three/choose among these/

Why?

> Toggle quote (7 lines)
>> +there are a few guiding principles to satisfy simultanuously where
>> +possible:
>> +
>> +@itemize
>> +@item
>> +In principle, Guix only has free software; when the upstream source
> 
> s/In principle, Guix only has free software/Every package in Guix is
> free software/g

This new sentence is most likely false.  There have been packages in 
Guix that were non-free and have been non-free, quite likely there will 
be non-free packages in Guix in the future.  I would rather have that 
Guix doesn't lie about itself.

>> +community (i.e., patterns that appear throughout @code{gnu/packages/...})
> 
> Normally such parenthetical expressions go between em dashes:
> 
>   community---i.e., patterns that appear throughout
>   @file{gnu/packages}---and …

Err, source?  Normally parenthetical expressions go between parentheses. 
  Also, normality is irrelevant.

>> +To make things more concrete and to resolve conflicts between the
>> +principles, a few cases have been worked out:
>> +
>> +@subsubsection Removing non-free software
>> +Non-free software has to be removed in snippets; the reason is that
>> +patches or phases will not work.
> 
> You can’t have a colon between the section heading.  Instead, you can
> write “a few cases have been worked out and will be illustrated in the
> following sections.”

You definitely can have a colon between the section heading, I just did 
so.  Also, that new sentence has a different meaning.

 > Section titles should be capitalized.

The letter R is capitalized.  I disagree that non-first words should be 
capitalized, but I suppose that's a style change that, if done, should 
be separate from this patch.

> Leave a blank line after the section title.

Why?

> Instead of “will not work”, which is vague, I’d suggest more explicit
> wording: “would not be appropriate because they would expose the
> offending code.”

To be more precise:

s/would not be appropriate/would be inappropriate

‘expose code’ and ‘offending code’ seems rather vague to me.  ‘will not 
work’ might be slightly vague, but it's clear from context (i.e., the 
words before the semicolon).  Also, didn't you have complaints about 
denseness?

>> +For patches, the problem is that a patch removing a non-free file
>> +automatically contains the non-free file@footnote{It has been noted that
>> +git patches support removing files without including the file in the
>> +patch in
>> +@url{https://yhetil.org/guix/8b13e899-eb60-490b-a268-639249698c81@@www.fastmail.com/}. If
>> +it is verified that the @command{patch} utility supports such patches,
>> +this method can be used and this policy adjusted appropriately.}, and we
>> +do not want anything non-free in Guix even if only in its patches.
> 
> I’d drop the footnote, it’s already dense enough.

I would rather keep the footnote -- there are no file size concerns for 
info files, it contains useful information, and it's a _footnote_.  One 
of the main benefits of footnotes is that they are out of the way and 
you can just skip them if you don't want to read them?

> s/snippets here:/snippets here./

Err, no? Directly after this sentence I'm saying which the benefits are, 
so this should be ':', not '.' -- ':' is strictly better here.

> s/When using snippets/First, when using snippets/

The next sentences don't do ‘Second, ...’, ‘Third, ...’, so no.

> Toggle quote (2 lines)
>> +As such, snippets are recommended here.
> 
> s/are recommended here/are the recommended way to delete non-free material/

No.  Please see see the subsubsection title:

 > +@subsubsection Removing bundled libraries

It's not merely about non-free material in in bundled libraries, it is 
about all bundled libraries.  It is not about all non-free material, but 
only about material in bundled libraries (unbundled non-free material 
are treated in the previous subsubsection).

As such, the proposed new sentence does not fit the subsubsection.

> Toggle quote (2 lines)
>> +@subsubsection Fixing technical issues (compilation errors, test failures, other bugs ...)
> 
> In addition to capitalizing, please remove the parenthetical bit.

No.  ‘Technical issues’ is vague, the parenthetical bit clarifies 
things.  Besides, in another comment for something else you wanted 
things to be less vague by adding additional text.

 > Could you send an updated version?

No.  The continued misunderstanding elsewhere in the thread and the 
inconsistency and occasional vagueness in arguments is tiresome.

Best regards,
Maxime Devos.