Hi Arun,
Arun Isaac <arunis...@systemreboot.net> writes:
> Hi Maxim,
>
> When a patch relates to no teams, I get the empty header output
> "X-Debbugs-Cc: ". For such patches, it would be nicer to not add any
> header instead of adding an empty header.
Good catch! Fixed.
>> +(define (team->members team)
>> + "Return the list of members in TEAM."
>> + (sort (team-members team)
>> + (lambda (m1 m2)
>> + (string<? (person-name m1) (person-name m2)))))
>> +
>> +(define (member->string member)
>> + "Return the 'email <name>' string representation of MEMBER."
>> + (format #false "~a <~a>" (person-name member) (person-email
>> member)))
>
> When person name contains a comma, it should be escaped by surrounding
> in double quotes. Else, the comma would interfere with the
> comma-separated list that we are constructing for
> X-Debbugs-Cc. Admittedly, none of our current team members have commas
> in their person names. But, some people like to have a name like
> "LastName, FirstName". We should protect against that edge case in the
> interest of futureproofing.
OK! I opted for simplicity and double-quoted all the names.
>> + (format #true "X-Debbugs-Cc: ~{~a~^,~}"
>> + (append-map (compose (cut map member->string <>)
>> + team->members
>> + find-team)
>> + (patch->teams patch-file))))
>
> A very nitpicky suggestion: Maybe, separate multiple persons by ", "
> instead of just ",". ", " looks slightly better cosmetically. Likewise
> in other places where this occurs.
Good idea; I've learnt this was valid email syntax just today :-).
Below is the diff of my rework:
--8<---------------cut here---------------start------------->8---
1 file changed, 18 insertions(+), 17 deletions(-)
etc/teams.scm.in | 35 ++++++++++++++++++-----------------
modified etc/teams.scm.in
@@ -605,20 +605,20 @@ (define (find-team-by-scope files)
(define (cc . teams)
"Return arguments for `git send-email' to notify the members of the given
TEAMS when a patch is received by Debbugs."
- (format #true
- "--add-header=\"X-Debbugs-Cc: ~{~a~^,~}\""
- (map person-email
- (delete-duplicates (append-map team-members teams) equal?))))
-
-(define (team->members team)
- "Return the list of members in TEAM."
- (sort (team-members team)
+ (let ((members (append-map team-members teams)))
+ (unless (null? members)
+ (format #true "--add-header=\"X-Debbugs-Cc: ~{~a~^, ~}\""
+ (map person-email (sort-members members))))))
+
+(define (sort-members members)
+ "Deduplicate and sort MEMBERS alphabetically by their name."
+ (sort (delete-duplicates members equal?)
(lambda (m1 m2)
(string<? (person-name m1) (person-name m2)))))
(define (member->string member)
"Return the 'email <name>' string representation of MEMBER."
- (format #false "~a <~a>" (person-name member) (person-email member)))
+ (format #false "\"~a\" <~a>" (person-name member) (person-email member)))
(define* (list-members team #:optional port (prefix ""))
"Print the members of the given TEAM."
@@ -626,7 +626,7 @@ (define* (list-members team #:optional port (prefix ""))
(for-each
(lambda (member)
(format port* "~a~a~%" prefix (member->string member)))
- (team->members team)))
+ (sort-members (team-members team))))
(define (list-teams)
"Print all teams, their scope and their members."
@@ -720,14 +720,15 @@ (define (main . args)
(apply cc (find-team-by-scope
(diff-revisions rev-start rev-end))))
(("cc-members-header-cmd" patch-file)
- (format #true "X-Debbugs-Cc: ~{~a~^,~}"
- (append-map (compose (cut map member->string <>)
- team->members
- find-team)
- (patch->teams patch-file))))
+ (let* ((teams (map find-team (patch->teams patch-file)))
+ (members (sort-members (append-map team-members teams))))
+ (unless (null? members)
+ (format #true "X-Debbugs-Cc: ~{~a~^, ~}"
+ (map member->string members)))))
(("cc-mentors-header-cmd" patch-file)
- (format #true "X-Debbugs-Cc: ~{~a~^,~}"
- (map member->string (team->members (find-team "mentors")))))
+ (format #true "X-Debbugs-Cc: ~{~a~^, ~}"
+ (map member->string
+ (sort-members (team-members (find-team "mentors"))))))
(("get-maintainer" patch-file)
(apply main "list-members" (patch->teams patch-file)))
(("list-teams" . args)
--8<---------------cut here---------------end--------------->8---
A proper v2 patch has been sent.
--
Thanks,
Maxim
