[PATCH] release/checks/whitespace: skip binary files

[PATCH] release/checks/whitespace: skip binary files

[Valentin Gagarin]

Trailing whitespace is meaningful only for text.

Signed-off-by: Valentin Gagarin <valentin@gagarin.work>
---
 release/checks/whitespace.nix | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/release/checks/whitespace.nix b/release/checks/whitespace.nix
index 56710b6..3929c8f 100644
--- a/release/checks/whitespace.nix
+++ b/release/checks/whitespace.nix
@@ -9,6 +9,6 @@ runCommand "spectrum-whitespace" {
     fileset = srcWithNix;
   };
 } ''
-  grep --color=always --exclude='*.patch' -r "[[:space:]]$" $src || touch $out
+  grep --color=always --binary-files=without-match --exclude='*.patch' -r "[[:space:]]$" $src || touch $out
 ''
 ) (_: {})
-- 
2.51.2

Re: [PATCH] release/checks/whitespace: skip binary files

[Alyssa Ross]

[-- Attachment #1: Type: text/plain, Size: 894 bytes --]

Valentin Gagarin <valentin@gagarin.work> writes:

> Trailing whitespace is meaningful only for text.
>
> Signed-off-by: Valentin Gagarin <valentin@gagarin.work>
> ---
>  release/checks/whitespace.nix | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/release/checks/whitespace.nix b/release/checks/whitespace.nix
> index 56710b6..3929c8f 100644
> --- a/release/checks/whitespace.nix
> +++ b/release/checks/whitespace.nix
> @@ -9,6 +9,6 @@ runCommand "spectrum-whitespace" {
>      fileset = srcWithNix;
>    };
>  } ''
> -  grep --color=always --exclude='*.patch' -r "[[:space:]]$" $src || touch $out
> +  grep --color=always --binary-files=without-match --exclude='*.patch' -r "[[:space:]]$" $src || touch $out
>  ''
>  ) (_: {})

Thanks — definitely an improvement.  I'm going to tweak to use the
shorter -I to keep the line length manageable.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]

Re: [PATCH] release/checks/whitespace: skip binary files

[Valentin Gagarin]

The reasons I wrote it out is that it's more obvious what it does 
without looking up the documentation.  As a project grows larger and so 
does the number of tools or options in use, the amount of rare things 
one needs to "just know" tends to become unmanageable, and the time to 
decompress a highly compressed representation goes up.

This is, what I consider to be by now classic, advice, which I have 
assumed to be tacit knowledge:
https://changelog.com/posts/use-long-flags-when-scripting

Discussion:
- https://news.ycombinator.com/item?id=5164354 (2013)
- https://news.ycombinator.com/item?id=24518682 (2020)

In this particular case, nothing would tell me off the cuff what `-I` is 
supposed to mean for `grep`.  It seems reasonable to be explicit here 
and avoid the extra round of distraction for readers.  This has even 
more impact on people with less experience (overall or just with the 
particular tools).

No action required now, the change is merged and does its thing.  But I 
suggest considering whether a note on the issue should be added to a 
document about coding conventions.

Re: [PATCH] release/checks/whitespace: skip binary files

[Alyssa Ross]

[-- Attachment #1: Type: text/plain, Size: 1358 bytes --]

Valentin Gagarin <valentin@gagarin.work> writes:

> The reasons I wrote it out is that it's more obvious what it does 
> without looking up the documentation.  As a project grows larger and so 
> does the number of tools or options in use, the amount of rare things 
> one needs to "just know" tends to become unmanageable, and the time to 
> decompress a highly compressed representation goes up.
>
> This is, what I consider to be by now classic, advice, which I have 
> assumed to be tacit knowledge:
> https://changelog.com/posts/use-long-flags-when-scripting
>
> Discussion:
> - https://news.ycombinator.com/item?id=5164354 (2013)
> - https://news.ycombinator.com/item?id=24518682 (2020)
>
> In this particular case, nothing would tell me off the cuff what `-I` is 
> supposed to mean for `grep`.  It seems reasonable to be explicit here 
> and avoid the extra round of distraction for readers.  This has even 
> more impact on people with less experience (overall or just with the 
> particular tools).
>
> No action required now, the change is merged and does its thing.  But I 
> suggest considering whether a note on the issue should be added to a 
> document about coding conventions.

I think you're probably right and I'd be happy for the convention to be
changed and documented (but long single lines still avoided).

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]

Long vs. short options

[Alyssa Ross]

[-- Attachment #1: Type: text/plain, Size: 3067 bytes --]

Alyssa Ross <hi@alyssa.is> writes:

> Valentin Gagarin <valentin@gagarin.work> writes:
>
>> The reasons I wrote it out is that it's more obvious what it does 
>> without looking up the documentation.  As a project grows larger and so 
>> does the number of tools or options in use, the amount of rare things 
>> one needs to "just know" tends to become unmanageable, and the time to 
>> decompress a highly compressed representation goes up.
>>
>> This is, what I consider to be by now classic, advice, which I have 
>> assumed to be tacit knowledge:
>> https://changelog.com/posts/use-long-flags-when-scripting
>>
>> Discussion:
>> - https://news.ycombinator.com/item?id=5164354 (2013)
>> - https://news.ycombinator.com/item?id=24518682 (2020)
>>
>> In this particular case, nothing would tell me off the cuff what `-I` is 
>> supposed to mean for `grep`.  It seems reasonable to be explicit here 
>> and avoid the extra round of distraction for readers.  This has even 
>> more impact on people with less experience (overall or just with the 
>> particular tools).
>>
>> No action required now, the change is merged and does its thing.  But I 
>> suggest considering whether a note on the issue should be added to a 
>> document about coding conventions.
>
> I think you're probably right and I'd be happy for the convention to be
> changed and documented (but long single lines still avoided).

I thought about this a bit more.  I think there are three classes of
scripts in Spectrum in regards to this, each with different
considerations:

• Bash code embedded into Nix files.  Being in Nix files they'll always
  be run in a consistent environment, so no problems with long options
  here.

• execline code that's installed into the Spectrum system.  As above,
  but it's worth noting that these scripts mostly run execline/s6
  executables, which do not provide any long options.  We could still
  use long options for programs that have them, but might feel
  inconsistent (although it's also inconsistent when long options need
  to be passed to some program today).  I think it probably makes sense
  to make the change here.

• Scripts that are part of Spectrum's build system.  These have always
  been written with the intention of being portable.  In some cases we
  might publish code as part of Spectrum that others might want to build
  on or for some other systems.  To facilitate reuse (and in the case of
  Make especially, also to try to avoid the pitfalls and complexity that
  come with the GNU extensions), I've discouraged use of non-standard
  features of utilities that might be part of an OS, like coreutils (or
  equivalent), or awk or Make.  POSIX only standardizes short options,
  so for this category, I think it makes sense to stick to those for the
  portability advantages.  We could further break this down into scripts
  that we expect to be reused and those we don't, but I think that would
  be a difficult distinction for people to make.

Just my thoughts. :)

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]

Re: [PATCH] release/checks/whitespace: skip binary files

[Alyssa Ross]

This patch has been committed as 4827f7591e945f331a945fb53a9fd4e8ebfdbc12,
which can be viewed online at
https://spectrum-os.org/git/spectrum/commit/?id=4827f7591e945f331a945fb53a9fd4e8ebfdbc12.

This is an automated message.  Send comments/questions/requests to:
Alyssa Ross <hi@alyssa.is>