Re: Encoding Problem in export?

[Nicolas Goaziou <n.goaziou@gmail.com>]

Hello,

Michael Brand <michael.ch.brand@gmail.com> writes:

> I would like to ask you to review the attached patch so I can change
> it when necessary before I git push it.

Sure.

> -	    (browse-url (concat type ":" (org-link-escape-browser path))))
> +	    ;; see `ert-deftest'
> +	    ;; `test-org/org-link-escape-chars-browser'
> +	    (browse-url
> +	     (if (fboundp 'url-encode-url)
> +		 (url-encode-url (concat type ":" path))
> +	       (org-link-escape-browser (concat type ":" path)))))

IMO, the following is nicer:

(funcall (if (fboundp 'url-encode-url) #'url-encode-url #'org-link-escape-browser)
         (concat type ":" path))

Also, it's better to document this in the source code rather than in the
test suite. Also, you could add, as a reminder, that we can remove
`org-link-escape-browser' altogether once we drop support for Emacs 23.

> -	    (browse-url (concat org-doi-server-url
> -				(org-link-escape-browser path))))
> +	    ;; see `ert-deftest'
> +	    ;; `test-org/org-link-escape-chars-browser'
> +	    (browse-url
> +	     (if (fboundp 'url-encode-url)
> +		 (url-encode-url (concat org-doi-server-url path))
> +	       (org-link-escape-browser (concat org-doi-server-url path)))))

Ditto.

> -  (should
> -   (string=
> -    "http://some.host.com/form?&id=blah%2Bblah25"
> -    (org-link-unescape
> -     (org-link-escape "http://some.host.com/form?&id=blah%2Bblah25")))))
> +  (let ((a "http://some.host.com/form?&id=blah%2Bblah25"))
> +    (should (string= a (org-link-unescape (org-link-escape a))))))

No need to change this. Moreover, I tend to prefer `should' outside the
sexp because it is easier to debug, when needed (`should' is quite
opaque when stepping through the function).

> +    ;; This is the behavior of `org-open-at-point' when used together
> +    ;; with an Emacs 24.3 or later where `url-encode-url' is available
> +    (when (fboundp 'url-encode-url)
> +      ;; "query="-space as plus sign
> +      (should (string= (concat query "%2Bsubject:%22Release+8.2%22")
> +		       (url-encode-url (concat query plus))))
> +      ;; "query="-space as space
> +      (should (string= (concat query "%2Bsubject:%22Release%208.2%22")
> +		       (url-encode-url (concat query space)))))

You are testing `url-encode-url' here, not an Org function. Is it really
required?


Regards,

-- 
Nicolas Goaziou