Add rudimentary support for working with the Syncthing API #4

Olivia5k · 2021-03-10T19:01:05Z

This lets us ask Syncthing for which directories we should search for conflicts
in. This also alters the emacs-conflict-resolve-conflicts call to make the
directory optional. If not provided, all directories owned by Syncthing are
scanned.

Adds Syncthing conflict cache. Since searching large (i.e. multi-gigabyte)
directories can take a while, the results are cached for a speedier workflow. A
emacs-conflict-syncthing-conflicts-refresh-cache command is provided to
refresh the cache. When a conflict is resolved its entry in the cache is
cleared.

Adds a emacs-conflict-remove-conflict that lets the user interactively clear
conflicts if they're invalid or otherwise impractical to handle via this
package. Large binary files can be rejected by ediff and is cumbersome to work
with in Emacs anyways.

Simple management of the Syncthing API key is added - it's prompted for if it is
not set. Also, since it's very plausible that a user that would be interested in
this package is also syncing their dotfiles already, there's support for
multiple API keys (keyed on the result of (system-name)). This was discovered
to be a problem during development of these features, heh.

Finally, add a keymap with the interactive features bound. A suggested bind for
it could be C-x c, with the conflict mnemonic.

This was written over two sessions a few months apart (started during the winter holidays, then finished it just now), but I hope it's still coherent. It seems to work nicely on my system here.

Some things to consider when reviewing:

Add `dash` as a library

Just because some things are easier to express with it. It's only used in a few places, so it might be overkill. I know that some package maintainers are keen to keep dependencies to a minimum, so if that's the desired case here I can probably rewrite it to only use native Emacs functionality.

Some default behavior was altered

The call to emacs-conflict-resolve-conflicts is altered to default to using the Syncthing sources if no directory is specified. The previous behavior was to prompt for a directory, and that functionality is now shadowed away. There are probably ways to make it available again, but I boldly assumed that the Syncthing integration would be the desirable choice anyway. However, if a user does not want to (or is unable to) use a version of Syncthing that enables this functionality (at least v.1.12.0) that leaves them... worse. I can probably rework it to support that, but I didn't put time into that for now.

The version was bumped

This might be something for a different commit made by the maintainer, so I thought I should give a heads up about including that now.

I'm not the most experienced package contributor, but I hope this is a useful contribution! This was fun to write, and it was my first time using ert during development! 😊

Oh, and of course, thanks for a useful and well-made package!

This lets us ask Syncthing for which directories we should search for conflicts in. This also alters the `emacs-conflict-resolve-conflicts` call to make the directory optional. If not provided, all directories owned by Syncthing are scanned. Adds Syncthing conflict cache. Since searching large (i.e. multi-gigabyte) directories can take a while, the results are cached for a speedier workflow. A `emacs-conflict-syncthing-conflicts-refresh-cache` command is provided to refresh the cache. When a conflict is resolved its entry in the cache is cleared. Adds a `emacs-conflict-remove-conflict` that lets the user interactively clear conflicts if they're invalid or otherwise impractical to handle via this package. Large binary files can be rejected by `ediff` and is cumbersome to work with in Emacs anyways. Simple management of the Syncthing API key is added - it's prompted for if it is not set. Also, since it's very plausible that a user that would be interested in this package is also syncing their dotfiles already, there's support for multiple API keys (keyed on the result of `(system-name)`). This was discovered to be a problem during development of these features, heh. Finally, add a keymap with the interactive features bound. A suggested bind for it could be `C-x c`, with the _conflict_ mnemonic.

ibizaman · 2021-03-22T09:40:08Z

Hi! Thanks for this great contribution!!! I’ll get to reviewing this shortly but got not that much time these days.

This is the silliest typo I've ever done. How did I... what. 😅

ibizaman

I really like this PR. But I envision this tool as handling more than just syncthing.
I don't exactly see how to merge my previous sentence with your PR nicely. I'm sure there's a way though and if you see, then I'd love for it to happen.
But if not, that's okay too. Could you then leave the interactive functions as-is and create a separate set for your use case?

ibizaman · 2021-04-23T07:31:35Z

emacs-conflict.el

+  (when (y-or-n-p (format "Delete conflict file %s? "
+                          (file-name-nondirectory conflict)))
+    ;; If there is no buffer, kill-buffer will be called with a nil argument,
+    ;; which means kill the current buffer - not what we want.


In which case are there no buffer?

ibizaman · 2021-04-23T07:32:43Z

emacs-conflict.el

+  :type 'string)
+
+(defcustom emacs-conflict-syncthing-api-keys nil
+  "Lookup for the API key used i making requests to Syncthing.


Suggested change

"Lookup for the API key used i making requests to Syncthing.

"Lookup for the API key used in making requests to Syncthing.

?

ibizaman · 2021-04-23T07:36:09Z

emacs-conflict.el

+all Syncthing instances.
+
+The API key be obtained in the GUI in the Settings window."
+  :type '(alist :key-type string :value-type string))


Do you have experience with https://www.gnu.org/software/emacs/manual/html_mono/auth.html?
Because here you're storing secrets in your emacs config.

In my case, I'm using https://www.passwordstore.org/ and emacs can load secrets from that transparently.

ibizaman · 2021-04-23T07:36:11Z

emacs-conflict.el

+  :type '(alist :key-type string :value-type string))
+
+(defvar emacs-conflict-syncthing-conflict-marker ".sync-conflict"
+  "The string used to denote that a file is a conflict file.")


I like this 👍 It paves the way for being able to handle more conflict markers.

ibizaman · 2021-04-23T07:40:50Z

emacs-conflict.el

+  "Gets all conflicts in Syncthing.
+
+The result is put into `emacs-conflict-syncthing-conflicts-cache'. This cache is
+  used unless empty or `emacs-conflict-syncthing-refresh-cache' is non-nil."


Instead of using a global variable emacs-conflict-syncthing-refresh-cache for refreshing the cache, could you instead add a default argument to the function?
I'm thinking something like this:

(defun emacs-conflict-syncthing-conflicts (&optional refresh-cache))

Then you can delete the emacs-conflict-syncthing-refresh-cache global variable.

ibizaman · 2021-04-23T07:41:38Z

emacs-conflict.el

+  "Denotes if we should query Syncthing or use the cache.
+
+Should only be set via `let' whenever we want to explcitly update the cache.
+Avoid `setq' on this otherwise.")


See comment above but I'd remove this variable.

ibizaman · 2021-04-23T20:23:23Z

emacs-conflict.el

+The object returned is the parsed JSON from the response."
+  (let* ((url (concat emacs-conflict-syncthing-url url))
+         (url-request-extra-headers (list (cons "X-API-key" (emacs-conflict-syncthing--api-key)))))
+    ;; TODO(thiderman): Maybe some error handling if a non-200 response is returned?


That would be nice indeed. For example, when trying the code I setup a wrong API key and got a cryptic error. We could at least print the raw response.

ibizaman · 2021-04-23T20:23:58Z

emacs-conflict.el

+      (customize-save-variable
+       'emacs-conflict-syncthing-api-keys
+       (acons (system-name) key emacs-conflict-syncthing-api-keys)))
+    key))


Maybe transitioning to auth-source like suggested above would be for the best.

ibizaman · 2021-04-23T21:46:51Z

emacs-conflict.el

+  (interactive
+   ;; TODO(thiderman): Only show roots that have conflicts?
+   (list (completing-read "Syncthing root: " (emacs-conflict-syncthing-roots))))
+  (find-name-dired directory (format "*%s-*" emacs-conflict-syncthing-conflict-marker)))


I see where you're getting at, and I like it, but I think it's going to be confusing to use. Usually, calling functions interactively or not does not change that much the behavior. Maybe having instead a separate function for each use case would be enough for now?

Fix typo

e15cb92

This is the silliest typo I've ever done. How did I... what. 😅

ibizaman reviewed Apr 23, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add rudimentary support for working with the Syncthing API #4

Add rudimentary support for working with the Syncthing API #4

Olivia5k commented Mar 10, 2021

ibizaman commented Mar 22, 2021

ibizaman left a comment

ibizaman Apr 23, 2021

ibizaman Apr 23, 2021

ibizaman Apr 23, 2021

ibizaman Apr 23, 2021 •

edited

Loading

ibizaman Apr 23, 2021

ibizaman Apr 23, 2021 •

edited

Loading

ibizaman Apr 23, 2021

ibizaman Apr 23, 2021

ibizaman Apr 23, 2021 •

edited

Loading

	"Lookup for the API key used i making requests to Syncthing.
	"Lookup for the API key used in making requests to Syncthing.

Add rudimentary support for working with the Syncthing API #4

Are you sure you want to change the base?

Add rudimentary support for working with the Syncthing API #4

Conversation

Olivia5k commented Mar 10, 2021

Add dash as a library

Some default behavior was altered

The version was bumped

ibizaman commented Mar 22, 2021

ibizaman left a comment

Choose a reason for hiding this comment

ibizaman Apr 23, 2021

Choose a reason for hiding this comment

ibizaman Apr 23, 2021

Choose a reason for hiding this comment

ibizaman Apr 23, 2021

Choose a reason for hiding this comment

ibizaman Apr 23, 2021 • edited Loading

Choose a reason for hiding this comment

ibizaman Apr 23, 2021

Choose a reason for hiding this comment

ibizaman Apr 23, 2021 • edited Loading

Choose a reason for hiding this comment

ibizaman Apr 23, 2021

Choose a reason for hiding this comment

ibizaman Apr 23, 2021

Choose a reason for hiding this comment

ibizaman Apr 23, 2021 • edited Loading

Choose a reason for hiding this comment

Add `dash` as a library

ibizaman Apr 23, 2021 •

edited

Loading

ibizaman Apr 23, 2021 •

edited

Loading

ibizaman Apr 23, 2021 •

edited

Loading