Re: [elpa] master aa5513d 1/7: excorporate-diary: Always use diary-fancy-display

Re: [elpa] master aa5513d 1/7: excorporate-diary: Always use diary-fancy-display

[Stefan Monnier]

> +  (unless (eq diary-display-function 'diary-fancy-display)
> +    (warn (format
> +	   (concat "Excorporate diary support needs diary-fancy-display"
> +		   " but diary-display-function is currently %S; overriding")
> +	   diary-display-function))
> +    (customize-set-variable 'diary-display-function 'diary-fancy-display))

The warning is good, but I think that unilaterally overriding the user's
choice is a bad idea.

For example, it prevents using a tweaked version of `diary-fancy-display`.


        Stefan

Re: [elpa] master aa5513d 1/7: excorporate-diary: Always use diary-fancy-display

[Thomas Fitzsimmons]

Hi,

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> +  (unless (eq diary-display-function 'diary-fancy-display)
>> +    (warn (format
>> +	   (concat "Excorporate diary support needs diary-fancy-display"
>> +		   " but diary-display-function is currently %S; overriding")
>> +	   diary-display-function))
>> +    (customize-set-variable 'diary-display-function 'diary-fancy-display))
>
> The warning is good, but I think that unilaterally overriding the user's
> choice is a bad idea.
>
> For example, it prevents using a tweaked version of `diary-fancy-display`.

Thanks for reviewing.  Yeah, I didn't like having to do that but the
stock alternative, diary-simple-display, is broken.  Specifically, even
with diary-include-other-diary-files in diary-list-entries-hook,
diary-simple-display doesn't show included entries.  Maybe that's
intended behavior for diary-simple-display, but it makes it seem like
Excorporate hasn't done anything (since all its entries are included
entries).

I'm not sure how to disallow diary-simple-display but allow tweaked
versions of diary-fancy-display.  (Currently I'm also relying on
diary-fancy-display-mode-hook.)  I suppose I could just leave the
warning and remove the customize-set-variable call.  I'll think about it
for the next release.

Maybe I should also file a bug about diary-simple-display not showing
included entries.

Thomas

Re: [elpa] master aa5513d 1/7: excorporate-diary: Always use diary-fancy-display

[Stefan Monnier]

> Thanks for reviewing.  Yeah, I didn't like having to do that but the
> stock alternative, diary-simple-display, is broken.

FWIW, the default is already the fancy display:

    % grep diary-display-function **/*.el
    [...]
    lisp/calendar/diary-lib.el:(defcustom diary-display-function #'diary-fancy-display

So if the user gets broken behavior it's presumably because of
a specific choice in its config, I think it's OK to just explain the
reason to the user without trying to impose a particular fix.


        Stefan

Re: [elpa] master aa5513d 1/7: excorporate-diary: Always use diary-fancy-display

[Thomas Fitzsimmons]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> Thanks for reviewing.  Yeah, I didn't like having to do that but the
>> stock alternative, diary-simple-display, is broken.
>
> FWIW, the default is already the fancy display:
>
>     % grep diary-display-function **/*.el
>     [...]
>     lisp/calendar/diary-lib.el:(defcustom diary-display-function #'diary-fancy-display
>
> So if the user gets broken behavior it's presumably because of
> a specific choice in its config, I think it's OK to just explain the
> reason to the user without trying to impose a particular fix.

Good point, pushed a change so that only the warning is issued.

Thomas