João Távora <joaotavora@gmail.com> writes:

> Danny Freeman <danny@dfreeman.email> writes:
>
>
>> +(defcustom eglot-report-progress t
>> +  "If non-nil, show progress of long running server work in the minibuffer."
>> +  :type 'boolean
>> +  :version "29.1")
>
> The usual question: is the version number here Eglot, the ELPA package's
> upcoming version number, or is it Emacs's upcoming version number.  I
> think Stefan Kangas had something to say about that.

Yeah, I've left this as in my updated patch. The answer is beyond my
qualifications :)

>
>> +
>> +            (message-log-max nil))
>
> I know that indentation was off here, and you fixed it.  But please
> revert this, and feel free to offer a separate cosmetic patch fixing
> this and other violations.

No problem. I have attached a second patch with only whitespace changes.
Feel free to take it or leave it. 

>
>>          (ignore-errors (delay-mode-hooks (funcall mode))))
>>        (font-lock-ensure)
>>        (string-trim (buffer-string)))))
>> @@ -2049,6 +2057,43 @@ eglot-handle-notification
>>    (_server (_method (eql telemetry/event)) &rest _any)
>>    "Handle notification telemetry/event.") ;; noop, use events buffer
>>  
>> +(defun eglot--progress-report-message (title message)
>> +  "Format a $/progress report message, given a TITLE and/or MESSAGE string."
>> +  (cond
>> +   ((and title message) (format "%s %s" title message))
>> +   (title title)
>> +   (message message)))
>> +
>> +(defun eglot--progress-reporter (server token)
>> +  "Get a prgress-reporter identified by the progress TOKEN from the SERVER ."
>> +  (cdr (assoc token (eglot--progress-reporter-alist server))))
>> +
>> +(defun eglot--progress-reporter-delete (server token)
>> +  "Delete progress-reporters identified by the progress TOKEN from the SERVER."
>> +  (setf (eglot--progress-reporter-alist server)
>> +        (assoc-delete-all token (eglot--progress-reporter-alist > server))))
>
> This is just a stylistic suggestion, but these functions could all be
> local inside the following eglot-handle-notification.  Then you could
> give them less mouthfully names.  The whole progress stuff could be
> examined by looking only at one function.

I have inlined them with cl-flet.

>> +
>> +(cl-defmethod eglot-handle-notification
>> +  (server (_method (eql $/progress)) &key token value)
>> +  "Handle a $/progress notification identified by TOKEN from the SERVER."
>> +  (when eglot-report-progress
>> +    (cl-destructuring-bind (&key kind title percentage message) value
>
> I think eglot-dbind is more appropriate here.  See the file for how it's
> used.

Done, that's a pretty neat macro.

>> +      (pcase kind
>> +        ("begin" (let* ((prefix (format (concat "[eglot] %s %s:" (when percentage " "))
>> +                                        (eglot-project-nickname server) token))
>> +                        (pr (if percentage
>> +                                (make-progress-reporter prefix 0 100 percentage 1 0)
>> +                              (make-progress-reporter prefix nil nil nil 1 0))))
>> +                   (eglot--progress-reporter-delete server token)
>> +                   (setf (eglot--progress-reporter-alist server)
>> +                         (cons (cons token pr) (eglot--progress-reporter-alist server)))
>> +                   (progress-reporter-update pr percentage (eglot--progress-report-message title message))))
>> +        ("report" (when-let ((pr (eglot--progress-reporter server token)))
>> +                    (progress-reporter-update pr percentage (eglot--progress-report-message title message))))
>> +        ("end" (when-let ((pr (eglot--progress-reporter server token)))
>> +                 (progress-reporter-done pr)
>> +                 (eglot--progress-reporter-delete server token)))))))
>
> This lines are a bit too long, I think.  Try to stick to 80 columns.
> M-x whitespace-mode helps in seeing that.

I've scaled back the width of the function a good bit. Some of the
parens still spill over in some places, happy to take a second stab at
it.

> I also think this could probably be simplified a bit or jumbled around
> to feel less repetitive.  But it's not really bad as it stands, so feel
> free to disregard.

I'm not sure what I could do to scale this back beyond what is there
now. If there is anything specific let me know.

>> +
>>  (cl-defmethod eglot-handle-notification
>>    (_server (_method (eql textDocument/publishDiagnostics)) &key uri diagnostics
>>             &allow-other-keys) ; FIXME: doesn't respect `eglot-strict-mode'
>> @@ -2172,7 +2217,7 @@ eglot--TextDocumentItem
>>    (append
>>     (eglot--VersionedTextDocumentIdentifier)
>>     (list :languageId
>> -	 (eglot--language-id (eglot--current-server-or-lose))
>> +         (eglot--language-id (eglot--current-server-or-lose))
>
> Same here re: indentation.

This was one of the lines that had a tab, I was more intentional with
the indentation in the whitespace patch.

>>           :text
>>           (eglot--widening
>>            (buffer-substring-no-properties (point-min) (point-max))))))
>
>
> The patch looks generally good: thanks!  If you want to do the fixes I
> suggested, go ahead.  Else give me some days to test it and I will
> push it.
>
> João

Thank you! Let me know if you have any other questions or feedback and I
will be happy to address it.

-- 
Danny Freeman