Re: [Emacs-diffs] master fd54102: * lisp/files.el (file-size-function): New defcustom

["Basil L. Contovounesios" <contovob@tcd.ie>]

Oleh Krehel <ohwoeowho@gmail.com> writes:

>> > [I wish changes like this (and its recent predecessors, which caused
>> >  test failues that others had to fix)
>
> Sorry about that. I didn't realize where the tests were, since there's
> no "make test" at top-level.

There's 'make check' and the file test/README, as mentioned under
"Testing your changes" in CONTRIBUTE.

>> > even if small, to central
>> >  user-facing features were instead discussed and tested a little before
>> >  being unilaterally pushed.]
>>
>> Seconded.
>
> While I agree in general, in this case the user-visible change was
> done around a year ago, and I noticed only now when I switched to
> using Emacs27. The change from a year ago was displaying the amount of
> free space in kilobytes, which is almost unusable.
> I added a custom var with a reasonable default that any user can
> easily change. Very uncontroversial change, IMO.

New user options in central places like files.el (as opposed to some
specialised package) usually warrant a RFC so as to establish which need
they are addressing, whether this need masks some other issue, and
whether the proposed change covers this need sufficiently well.

Internal variables can come and go, but user options are user-facing
contracts that are harder to change or get rid of.

>> > Did you try radio+function-item instead of choice+const?  I usually find
>> > the former nicer, as recommended in (info "(elisp) Composite Types").
>
> OK.
>
>> > > +          (const :tag "default" file-size-human-readable)
>> > > +          (const :tag "iec"
>> >
>> > Nit: Please capitalise and uppercase these tags, respectively.
>
> OK.
>
>> > > +           (lambda (size) (file-size-human-readable size 'iec " ")))
>> >
>> > Please do not use an unevaluated anonymous function here.
>
> OK.

Further to [1], I should clarify that anonymous functions shouldn't be
used as values in a user option's :type at all, regardless of whether
the function is evaluated.

[1]: * lisp/files.el (file-size-function): Add :version tag
45fc6f203e 2019-07-20 19:31:07 +0200
https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=45fc6f203e2fef528cb2bb0d7c0140e160c974e2

>> This defcustom also needs a :version tag.
>
> OK. I've pushed the requested changes.

Thanks,

-- 
Basil