emacs-orgmode@gnu.org archives
 help / color / mirror / code / Atom feed
From: Nicolas Goaziou <mail@nicolasgoaziou.fr>
To: "Глеб Бородуля" <mr.reg@mail.ru>
Cc: emacs-orgmode@gnu.org
Subject: Re: contrib/lisp/ox-taskjuggler.el patch
Date: Mon, 29 Jun 2015 10:05:59 +0200	[thread overview]
Message-ID: <87vbe7kkgo.fsf@nicolasgoaziou.fr> (raw)
In-Reply-To: <1434905740.963429258@f367.i.mail.ru> ("Глеб Бородуля"'s message of "Sun, 21 Jun 2015 19:55:40 +0300")

Hello,

Глеб Бородуля <mr.reg@mail.ru> writes:

> I want to pull some changes in contrib/lisp/ox-taskjuggler.el file
> There is patch in attach, created with git format-patch
> Manually written change log here:

Thank you for the patch. Comments follow.

> ox-taskjuggler.el: Minor enhancements in taskjuggler export

It would be better to split these enhancements into as many patches.

> * contrib/lisp/ox-taskjuggler.el b/contrib/lisp/ox-taskjuggler.el
> (org-taskjuggler--build-project): added new custom variable
> `org-taskjuggler-default-project-configuration' that can be used to
> configure additional project fields.

You need to capitalize the sentence. Also, it looks like you're using an
outdated "ox-taskjuggler.el". See below.

> (org-taskjuggler-get-end): added support for `end' property instead
> only standard org deadline value.

Ditto.

> (org-taskjuggler--build-task): added support for multiple allocations,
> specified by separators.

Not sure about the usefulness of this one. According to
<http://www.taskjuggler.org/tj3/manual/allocate.html>,

  allocate task1, task2, task3

is valid. There's apparently no need to split the task list.

> -    (and deadline (org-timestamp-format deadline "%Y-%02m-%02d"))))
> +    (or
> +     (and deadline (org-timestamp-format deadline "%Y-%02m-%02d"))
> +     (and (memq 'end org-taskjuggler-valid-task-attributes)
> +	  (org-element-property :END item)))))

You don't need to check `org-taskjuggler-valid-task-attributes' here
since you're not applying it to a task.

>  channel.  If no start date is specified, start today.  If no end
>  date is specified, end `org-taskjuggler-default-project-duration'
>  days from now."
> -  (format "project %s \"%s\" \"%s\" %s %s {\n}\n"
> +  (format "project %s \"%s\" \"%s\" %s %s {%s\n}\n"
>            (org-taskjuggler-get-id project info)
>            (org-taskjuggler-get-name project)
>            ;; Version is obtained through :TASKJUGGLER_VERSION:
> @@ -726,7 +734,10 @@ days from now."
>                (format-time-string "%Y-%m-%d"))
>            (let ((end (org-taskjuggler-get-end project)))
>              (or (and end (format "- %s" end))
> -                (format "+%sd" org-taskjuggler-default-project-duration)))))
> +                (format "+%sd" org-taskjuggler-default-project-duration)))
> +	  (mapconcat
> +	   'org-element-normalize-string
> +	   org-taskjuggler-default-project-configuration "")))

Current definition for org-taskjuggler--build-project is

  (concat
     ;; Opening project.
     (format "project %s \"%s\" \"%s\" %s %s {\n"
  	   (org-taskjuggler-get-id project info)
  	   (org-taskjuggler-get-name project)
  	   ;; Version is obtained through :TASKJUGGLER_VERSION:
  	   ;; property or `org-taskjuggler-default-project-version'.
  	   (or (org-element-property :VERSION project)
  	       org-taskjuggler-default-project-version)
  	   (or (org-taskjuggler-get-start project)
  	       (format-time-string "%Y-%m-%d"))
  	   (let ((end (org-taskjuggler-get-end project)))
  	     (or (and end (format "- %s" end))
  		 (format "+%sd"
  			 org-taskjuggler-default-project-duration))))
     ;; Add attributes.
     (org-taskjuggler--indent-string
      (org-taskjuggler--build-attributes
       project org-taskjuggler-valid-project-attributes))
     ;; Closing project.
     "}\n")

Is `org-taskjuggler-default-project-configuration' needed considering
the attributes part above?

> -          (format "  purge %s\n  allocate %s\n"
> -                  ;; Compatibility for previous TaskJuggler versions.
> -                  (if (>= org-taskjuggler-target-version 3.0) "allocate"
> -                    "allocations")
> -                  allocate))
> +       (concat (format "  purge %s\n"
> +		       ;; Compatibility for previous TaskJuggler versions.
> +		       (if (>= org-taskjuggler-target-version 3.0) "allocate"
> +			 "allocations"))
> +	       ;;support for several allocations divided by separator
> +	       (mapconcat #'(lambda (allocation)
> +			      (format "  allocate %s\n" allocation))
> +			  (org-split-string allocate "[ ,]* +")
> +			  "")))

Is it really useful? Also, there's no need for #' in front of a lambda.


Regards,

-- 
Nicolas Goaziou

      reply	other threads:[~2015-06-29  8:04 UTC|newest]

Thread overview: 2+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-06-21 16:55 contrib/lisp/ox-taskjuggler.el patch Глеб Бородуля
2015-06-29  8:05 ` Nicolas Goaziou [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: https://www.orgmode.org/

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87vbe7kkgo.fsf@nicolasgoaziou.fr \
    --to=mail@nicolasgoaziou.fr \
    --cc=emacs-orgmode@gnu.org \
    --cc=mr.reg@mail.ru \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs/org-mode.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).