emacs-orgmode
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [O] [PATCH] org-protocol: fixes open-source and extends rewriting of


From: Nicolas Goaziou
Subject: Re: [O] [PATCH] org-protocol: fixes open-source and extends rewriting of URLs
Date: Wed, 28 Jun 2017 11:32:50 +0200
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/25.2 (gnu/linux)

Hello,

Mario Martelli <address@hidden> writes:

> Sorry for producing noise. Just read the contribution guidelines once again ;)
> Hope everything is now according to the guidelines.

Thank you. Comments follow.

> Subject: [PATCH 1/4] org-protocol.el: Fix for failing open-source subprotocol
>
> *  (org-protocol-open-source): make sure url is
> sanitised before processing

Applied.

> Subject: [PATCH 2/4] org-protocol.el: Fix for silently failing open-source
>  subprotocol
>
> * (org-protocol-open-source): tests URL against
> base-url and not the filename

Applied.

> Subject: [PATCH 3/4] org-protocol.el sources with date URL are supported
>
> * (org-protocol-project-alist): date-URL is added as example
> * (org-protocol-open-source): first match is processed in rewrite

Not applied. See below.

> +          :rewrites ((\"org/?$\" . \"index.php\")))
> +         (\"Hugo based blog\"
> +          :base-url \"https://themes.gohugo.io/theme/hugo-icarus/\";
> +          :working-directory 
> \"~/Documents/MyBlog/themes/hugo-icarus-theme/exampleSite/content/post/\"
> +          :online-suffix \".html\"
> +          :working-suffix \".md\"
> +          :rewrites 
> ((\"\\(https://themes.gohugo.io/theme/hugo-icarus/[0-9]+/[0-9]+/[0-9]+/\\)\" 
> . \".md\")))))

Would it be possible to make the path involved smaller so that the
example can fit in 80 columns?

>     The last line tells `org-protocol-open-source' to open
>     /home/user/org/index.php, if the URL cannot be mapped to an existing
> @@ -554,8 +561,13 @@ The location for a browser's bookmark should look like 
> this:
>                     ;; Try to match a rewritten URL and map it to
>                     ;; a real file.  Compare redirects without
>                     ;; suffix.
> -                   (when (string-match-p (car rewrite) f1)
> -                     (throw 'result (concat wdir (cdr rewrite))))))))
> +                   (when (string-match (car rewrite) f1)
> +                     (setq replacement (cdr rewrite))

Here, `replacement' isn't lexically bound, so compiling
"org-protocol.el" should return an error. `replacement' should be
let-bound instead.

> +                     (if (match-string 0 f1)

AFAIU, (match-string 0 f1) is necessarily true, per the (string-match
(car rewrite) f1) above, so the test can be removed.

> +                         (setq replacement (concat
> +                                            (directory-file-name  
> (replace-match "" nil nil f1 1))
> +                                            replacement )))))

Spurious white space character before the parenthesis.

> +                 (throw 'result (concat wdir replacement)))))

Shouldn't the (throw ...) be within the (when (string-match ...)) as it
was before? I may be wrong. I didn't check the high level logic of the
patch since I don't know "org-protocol.el".

> Applied for it just now. Have marked the changes as tiny.

Great. Let me know when the process is complete.

Regards,

-- 
Nicolas Goaziou                                                0x80A93738



reply via email to

[Prev in Thread] Current Thread [Next in Thread]