Some notes about magit--refresh-cache

[tarsius]

You might want to use magit--refresh-cache instead of rolling your own magithub-cache--refreshed-forms.

Note however how in this case cache lookup is done shortly before the value would be calculated by other means, while your cache has to be explicitly used by every caller. I think Magit's approach is superior, but it could be rather difficult to change your code to do it like that because you work a lot with wrappers such as magithub-request and magithub-unpaginate. The right place to implement this in my opinion would be in ghubp-request. Somethink like (untested):

(defun ghubp-request (method resource params data)
  ;; TODO limit use of cache to CLASS = magithub?
  ;; TODO limit use of cache to METHOD = get or post (for graphql)
  (let-alist (ghubp-get-context)
    (let ((key (list method resource params data
                     .unpaginate .headers .username .auth .host)))
      (magit--with-refresh-cache key
        ;; ^ Either use the value previously used for KEY during this refresh,
        ;; v or evaluate the body to get it.
        (magithub--with-cache key ; TODO class message after-update
          ;; ^ Either use the value from the cache,
          ;; V or evaluate the body to retrieve it.
          (let ((method (encode-coding-string (upcase (symbol-name method)) 'utf-8))
                (params (apiwrap-plist->alist params)))
            (funcall (or ghubp-request-override-function
                         #'ghub-request)
                     method resource nil
                     :query params
                     :payload data
                     :unpaginate .unpaginate
                     :headers .headers
                     :username .username
                     :auth .auth
                     :host .host)))))))

(defmacro magit--with-refresh-cache (key &rest body)
  ;; Obviously this is oversimplified, see `magit--with-refresh-cache'
  ;; and its callers for inspiration.
  `(or (and (magithub--use-cache-p ...)
            (magithub--get-cached-value ...))
       (let ((value (progn ,@body)))
         (when value
           (magithub--cache-value key value))
         value)))

[vermiculus]

I will think on this. If caching should be done at the ghubp-request level, then it should probably exist within ghub+ itself. I'd like to keep ghub+ as general-use as possible (though I wouldn't immediately expect it to be used by other packages at this stage of the game, even as relatively stable as it is) and this seems contraindicated by the need for a magit-specific caching solution.

Although… ghubp-request-override-function exists. It was originally created to effect reliable unit testing, however that would be a good approach to this as well. Every ghub request should already be wrapped in the magithub-request macro – perhaps this macro can be expanded upon into come magithub-cached-request that uses the functionality you've outlined here. You can tell I'm tired… this is not how that variable works 😄 but something similar could easily be written.

I probably won't be able to act on this until the weekend, but thanks for your notes; they're always appreciated 😄

[tarsius]

It's probably not worth doing this anymore.