[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [O] [PATCH] org-protocol: Allow optional port specification
From: |
Rasmus |
Subject: |
Re: [O] [PATCH] org-protocol: Allow optional port specification |
Date: |
Wed, 02 Dec 2015 20:32:32 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/25.1.50 (gnu/linux) |
Hi Sacha,
Thanks for your patch.
Sacha Chua <address@hidden> writes:
> I was trying to get org-protocol to work on KDE Plasma 5.4.2. I set up
> my ~/.kde/share/kde4/services/org.protocol, but the standard
> org-protocol sample syntax:
>
> org-protocol://store-link://URL/TITLE
>
> resulted in the error:
>
> Malformed URL
> Port field was empty; source was "..."; scheme = "org-protocol",
> host = "store-link", path = "// ..."
>
> Modifying my Javascript to create links of the form:
>
> org-protocol://store-link:0//URL/TITLE
>
> made org-protocol correctly pass the link to emacsclient KDE 5.4.2. This
> patch allows the optional specification of a port in the URI. What do
> you think?
First, I’m not familiar with org-protocol or messaging systems in general
so I cannot offer input on this.
> @@ -532,7 +532,7 @@ as filename."
> (when (string-match the-protocol fname)
> (dolist (prolist sub-protocols)
> (let ((proto (concat the-protocol
> - (regexp-quote (plist-get (cdr prolist)
> :protocol)) ":/+")))
> + (regexp-quote (plist-get (cdr prolist)
> :protocol)) ":[^/]*/+")))
This seems pretty general. Are there any dangerous of accepting
everything but /? Is this only meant to capture a port? If so, is there
any disadvantage in only allowing numbers? As said, I don’t know anything
about these topics.
> +;;; test-org-protocol.el --- tests for org-protocol.el
Tests
Also, I guess you should add -*- lexical-binding: t; -*- these days.
> +;;; Code:
> +
> +(ert-deftest test-org-protocol/org-protocol-check-filename-for-protocol ()
> + "Test `org-protocol-check-filename-for-protocol' specifications."
> + ;; Store link
> + (let ((uri "/some/directory/org-protocol:/store-link:/URL/TITLE"))
> + (should (null (org-protocol-check-filename-for-protocol uri (list uri)
> nil))))
> + (should (equal (car org-stored-links) '("URL" "TITLE")))
> + ;; Handle multiple slashes
> + (let ((uri "/some/directory/org-protocol://store-link://URL2//TITLE2"))
> + (should (null (org-protocol-check-filename-for-protocol uri (list uri)
> nil))))
> + (should (equal (car org-stored-links) '("URL2" "TITLE2")))
> + ;; Ignore port - useful for KDE
> + (let ((uri "/some/directory/org-protocol:/store-link:0//URL3//TITLE3"))
> + (should (null (org-protocol-check-filename-for-protocol uri (list uri)
> nil))))
> + (should (equal (car org-stored-links) '("URL3" "TITLE3"))))
I don't know org-protocol well enough to comment on your tests. But I
guess you should add something like this, extrapolating from other test
files,
(unless (featurep 'org-protocol)
(signal 'missing-test-dependency "org-protocol"))
Cheers,
Rasmus
--
⠠⠵
- [O] [PATCH] org-protocol: Allow optional port specification, Sacha Chua, 2015/12/02
- Re: [O] [PATCH] org-protocol: Allow optional port specification,
Rasmus <=
- Re: [O] [PATCH] org-protocol: Allow optional port specification, Aaron Ecay, 2015/12/03
- [O] [PATCH] org-protocol: Allow key=val&key2=value2-style URLs, Sacha Chua, 2015/12/04
- Re: [O] [PATCH] org-protocol: Allow key=val&key2=value2-style URLs, Aaron Ecay, 2015/12/05
- Re: [O] [PATCH] org-protocol: Allow key=val&key2=value2-style URLs, Sacha Chua, 2015/12/07
- Re: [O] [PATCH] org-protocol: Allow key=val&key2=value2-style URLs, Sacha Chua, 2015/12/07
- Re: [O] [PATCH] org-protocol: Allow key=val&key2=value2-style URLs, Sacha Chua, 2015/12/18
- Re: [O] [PATCH] org-protocol: Allow key=val&key2=value2-style URLs, Nicolas Goaziou, 2015/12/20
- Re: [O] [PATCH] org-protocol: Allow key=val&key2=value2-style URLs, Sacha Chua, 2015/12/21
- Re: [O] [PATCH] org-protocol: Allow key=val&key2=value2-style URLs, Nicolas Goaziou, 2015/12/22