Re: ELPA: New package: nano-theme

[Philip Kaludercic]

Philip Kaludercic <philipk@posteo.net> writes:

> "Nicolas P. Rougier (inria)" <nicolas.rougier@inria.fr> writes:
>
>> Dear all,
>>
>> I would like to submit a new package to ELPA which is a theme based on
>> Material colors (https://material.io/) for the light version and Nord
>> colors (https://www.nordtheme.com/) for the dark version. The theme
>> itself is made of (only) 6 different faces and the rationale is here:
>> https://arxiv.org/abs/2008.06030
>>
>> The theme is hosted at https://github.com/rougier/nano-theme and the
>> README displays what it looks like. Note that the theme exploits face
>> inheritance a lot and Eli explained me that it might degrade
>> performances. I don't know if this needs to be fixed.
>
> I see that a lot of faces inherit from nano-default, nano-strong,
> nano-faded, etc. Wouldn't it make more sense to inherit from built-in
> faces and then let default, bold, shadow, etc. inherit from these faces.
>
> Furthermore, I wonder why you define the commands nano-light and
> nano-dark, instead of two themes, nano-light and nano-dark (along the
> same lines of what modus-themes currently does). You could also turn
> nano-setup into a custom theme, so that the user can easily enable and
> disable it.

I see nano-theme was added despite still providing functions for themes,
instead of using customize (and a few other points I raised). Have I
missed an explanation as to why this decision was made? Did it turn out
to not be possible? It seems regrettable to provide a theme on ELPA that
evades the conventional theme infrastructure, especially considering
that ELPA currently doesn't provide that many themes to begin with.

> A few more points:
>
> - Should the font really be part of the face? I think it makes sense to
>   recommend a few fonts, but it is unconventional to add it as part of a
>   theme.
> - When you are only using one branch of a if expression, prefer
>   when. Instead of (if (not ...) ...), prefer unless.
> - Lines 363ff. seem to be indented unconventionally. Maybe add a
>   .dir-locals.el to make sure everyone is using the same whitespace
>   configuration.
>
> Also, as you depend on at least 27.1, you probably don't have to check
> if custom-theme-load-path is defined. The same applies to tooltip-mode,
> scroll-bar-mode and tool-bar-mode. What you are doing looks more like
> configuration code, where you want to make sure that a .emacs works on
> older versions of Emacs.
>
>> Best,
>> Nicolas

-- 
        Philip Kaludercic