Protect board and card name when building prefix-log.

[PierreTechoueyres]

Replace the percent sign (%) in the names of cards or cards to avoid an error message when orgtrello-log-msg is called.
I started make test and all the tests are correct, but I did not find how to add new ones for the current case.

ex:
prepare a card with text as below

* a card with a percentage as in 45%
Some text ...

try adding a comment with C-c o C, type some text and validate with C-c C-c
You have the following error message:
` org-trello - Add a comment on the card '5abac66544c84eb1a898082e' ... FAILED. Error: ("Not enough arguments for format string" error) `

This patch corrects this behavior.

[coveralls]

Coverage decreased (-0.1%) to 98.18% when pulling 859a062 on PierreTechoueyres:pte/percent into c38c361 on org-trello:master.

[coveralls]

Coverage decreased (-0.1%) to 98.18% when pulling 859a062 on PierreTechoueyres:pte/percent into c38c361 on org-trello:master.

[ardumont]

Hello, nice catch.

That sounds more like a behavior to fix in the orgtrello-log-msg function.

Possible implementation:

1. Checking(or mapping directly the replace you implemented) for that particular symbol in all string input and replace it if present.
2. Executing the actual log implementation and trap the error. When the particular error is caught, try to convert as in 1.

Either way, we would then deal for all actual org-trello log messages.

Also, that possibly is simpler to capture in tests as it centralizes that behavior to check for that particular function.

As a workflow, i'd probably do something:

• add a test prior to implementation on the orgtrello-log-msg function, this test would fail in the 0.8.1 base code with input holding a string with % in it.
• Fix according to 1 or 2 or something even better not demonstrated here ;)

As to implementation, that would possibly be around either error handling or throw/catch functions, as demo below (without actually testing it works though):

Actual code (org-trello-log.el file):

(defun orgtrello-log-msg (level &rest args)
  "Log message with LEVEL.
Depending on `orgtrello-log-level', this will be displayed or not.
All errors are displayed anyway.
ARGS constitutes the parameters to feed to message."
  (when (or (<= level orgtrello-log-level) (eq orgtrello-log-error level))
    (apply 'message (format "org-trello - %s" (car args)) (cdr args))))

could possibly be improved to:

(defun orgtrello-log--sanitize-input (arg)
  "Sanitize input by protecting against specific character in ARG.
Basic implementation, we could actually check the symbol exists
in it.
ARG constitutes the arg to sanitize, could be anything loggable.
"
  (if (stringp arg)
      (replace-regexp-in-string "%" "%%" arg)
    arg))

;; 1st version
(defun orgtrello-log-msg-1 (level &rest args)
  "Log message with LEVEL.
Depending on `orgtrello-log-level', this will be displayed or not.
All errors are displayed anyway.
ARGS constitutes the parameters to feed to message."
  (-let ((_args (mapcar orgtrello-log--sanitize-input args)))
    (when (or (<= level orgtrello-log-level) (eq orgtrello-log-error level))
      (apply 'message (format "org-trello - %s" (car args)) (cdr args)))))

;; 2nd version
(defun orgtrello-log-msg (level &rest args)
  "Log message with LEVEL.
Depending on `orgtrello-log-level', this will be displayed or not.
All errors are displayed anyway.
ARGS constitutes the parameters to feed to message."
  (when (or (<= level orgtrello-log-level) (eq orgtrello-log-error level))
    (condition-case nil
        (apply 'message (format "org-trello - %s" (car _args)) (cdr _args))
      (error
       (let ((args (mapcar orgtrello-log--sanitize-input args)))
         (apply 'message (format "org-trello - %s" (car args)) (cdr args)))))))

Note: again, not tested, only linted ;)

What do you think?

Cheers,

[PierreTechoueyres]

Hello @ardumont,
I first thinked like you : solve it at orgtrello-log-msg level. But the problem is that the signature of this function is : (defun orgtrello-log-msg (level &rest args). So at this level you can't distinct between the folowing calls:

1. (orgtrello-log-msg orgtrello-log-info "Hi there at %s" (format-time-string "%Y/%m/%d %H:%M:%S")) => org-trello - Hi there at 2018/03/28 22:09:01
2. (orgtrello-log-msg orgtrello-log-info "Hi there ! you've done 95% of the work" ) should be org-trello - Hi there ! you’ve done 95% of the work but you get an error (error "Format specifier doesn’t match argument type")
3. (orgtrello-log-msg orgtrello-log-info "Hi there ! you've done 95% of the work at %s" (format-time-string "%Y/%m/%d %H:%M:%S")) => (error "Format specifier doesn’t match argument type") should be org-trello - Hi there ! you’ve done 95% of the work at 2018/03/28 22:11:16

So I tried to send "good" informations to orgtrello-log-msg.

[ardumont]

So at this level you can't distinct between the folowing calls:...

Right.
Somehow that bugs me though!

2....
3....

That's unexpected!
Nice catch.

I did not get why those calls failed but the message function call underneath fails as well, that might be it.

(message "Hi there ! you've done 95% of the work")
;; raise Debugger entered--Lisp error: (error "Not enough arguments for format string")

So, that made me adapt slightly my proposal.
I changed the orgtrello-log--sanitize-input's implementation (based on yours ;)
Now the problems mentioned is no longer.

(defun orgtrello-log--sanitize-input (arg)
  "Sanitize input by protecting against specific character in ARG.
Basic implementation, we could actually check the symbol exists
in it.
ARG constitutes the arg to sanitize, should something loggable."
  (if (stringp arg)
      (replace-regexp-in-string "% " "%% " arg)  ;; I changed the regexp to only apply on '%' followed by a space.
    arg))

(defun orgtrello-log-msg-1 (level &rest args)
  "Log message with LEVEL.
Depending on `orgtrello-log-level', this will be displayed or not.
All errors are displayed anyway.
ARGS constitutes the parameters to feed to message."
  (when (or (<= level orgtrello-log-level) (eq orgtrello-log-error level))
    (condition-case nil
        (apply 'message (format "org-trello - %s" (car args)) (cdr args))
      (error
       (let ((args (mapcar 'orgtrello-log--sanitize-input args)))
         (apply 'message (format "org-trello - %s" (car args)) (cdr args)))))))

(defun orgtrello-log-msg-2 (level &rest args)
  "Log message with LEVEL.
Depending on `orgtrello-log-level', this will be displayed or not.
All errors are displayed anyway.
ARGS constitutes the parameters to feed to message."
  (when (or (<= level orgtrello-log-level) (eq orgtrello-log-error level))
    (let ((args (mapcar 'orgtrello-log--sanitize-input args)))
      (apply 'message (format "org-trello - %s" (car args)) (cdr args)))))

;; those actually works now so \m/
(orgtrello-log-msg-1 orgtrello-log-info "done 95% of work")
(orgtrello-log-msg-1 orgtrello-log-info "Hi there at %s" (format-time-string "%Y/%m/%d %H:%M:%S"))
(orgtrello-log-msg-1 orgtrello-log-info "Hi there ! you've done 95% of the work" )
(orgtrello-log-msg-2 orgtrello-log-info "Hi there ! you've done 95% of the work at %s" (format-time-string "%Y/%m/%d %H:%M:%S"))
(orgtrello-log-msg-2 orgtrello-log-info "done 95% of work")
(orgtrello-log-msg-2 orgtrello-log-info "Hi there at %s" (format-time-string "%Y/%m/%d %H:%M:%S"))
(orgtrello-log-msg-2 orgtrello-log-info "Hi there ! you've done 95% of the work" )
(orgtrello-log-msg-2 orgtrello-log-info "Hi there ! you've done 95% of the work at %s" (format-time-string "%Y/%m/%d %H:%M:%S"))

What do you think?

I prefer version orgtrello-message-2 (which evaluates args only when the log level is correct, it's better than the proposed 2's first version which evaluated even when not needed).

Cheers,

[PierreTechoueyres]

Looks better, but I don't know if you looked at Percent sign. There is one language (Hebrew) where the percent sign precede the number and without a space ...
On the other hand, you could decide to exclude officially this case.
And indeed with the version orgtrello-message-2 you only replace problematic characters.

[ardumont]

Looks better, but I don't know if you looked at Percent sign. There is one language (Hebrew) where the percent sign precede the number and without a space ...

Oh, Interesting, thanks for sharing!

On the other hand, you could decide to exclude officially this case.

That sounds reasonable. After all, I don't intend to translate in hebrew (not even in my language really ;).
And if someone wants to, we'll deal with that when the need arise.

And indeed with the version orgtrello-message-2 you only replace problematic characters.

Well, both version, it's just 2 is more concise and looks more functional to me.

--

So, how do you want to proceed, if at all?

• do you still want to contribute? meaning adapting this PR to the implementation 2, then add some tests around this.
• Or do you want to close this PR, and then I update org-trello accordingly.

Either way sounds good to me ;)

Thanks for your time.

Cheers,

[PierreTechoueyres]

I think it's more your solution than my PR now :-)
So you could close this PR and update org-trello.

[ardumont]

I think it's more your solution than my PR now :-)

I believe it's a joint effort ;)

So you could close this PR and update org-trello.

Ok, done.

Thanks for raising this up!