Re: [O] Starting source code export at non-zero (-n value)

[Brian Carlson]

On 2016-05-20 16:48, Nicolas Goaziou wrote:
> Hello,
>
> Brian Carlson <address@hidden> writes:
>
> > Hello other org mode aficionados! (I apologize for the errant email I
> > send a minute ago. )

> >  From 6b4db0a978cc3492f0d0ac7e29008de6846fbe4a Mon Sep 17 00:00:00 2001
> > From: Brian Carlson <address@hidden>
> > Date: Mon, 16 May 2016 10:58:01 -0400
> > Subject: [PATCH] ox: provide [+-]n <offset> functionality
>
> Here you need to specify all the functions being modified. You may want
> to look at other commit messages in the repository.

I'll make the appropriate commentary in my commit message in my branch.

> > @@ -1488,9 +1488,9 @@ contextual information."
> >            (custom-env (and lang
> >                             (cadr (assq (intern lang)
> >                                         org-groff-custom-lang-environments))))
> > -         (num-start (case (org-element-property :number-lines src-block)
> > +         (num-start (case (car (org-element-property :number-lines src-block))
> >                         (continued (org-export-get-loc src-block info))
> > -                      (new 0)))
> > +                      (new (or (cdr (org-element-property :number-lines 
> > src-block)) 0))))
>
> This pattern appears often in your patch, and, of course in the code
> base. I suggest to factorize it out.  Indeed, we could take advantage of
> the new behaviour of `org-export-get-loc' that your patch introduces.

I started doing that, and with your recommendation. I'll do just that.

> IIUC, the new `org-export-get-loc' returns the number for the first line
> of the current block, not the number of the last line in the previous
> block, like it used to do. It can then includes the construct above:
>
>    (pcase (org-element-property :number-lines src-block)
>      ;; No need to compute line numbers before this one.
>      (`(new . ,n) (or n 2))
>      (`(continued . ,_)
>       ;; Count all lines above, up to the first block that has "new" line
>       ;; numbering.
>       (let ((loc 0))
>         (org-element-map (plist-get info :parse-tree)
>             ...))))
>
> Of course, tests would have to be changed or updated accordingly.

A very nice recommendation. I'll look into this.

> > -                       ((string-match "-n\\>" switches) 'new)
> > -                       ((string-match "+n\\>" switches) 'continued)))
> > +                       ((string-match "-n *\\([0-9]+\\)" switches) (cons 'new 
> > (- (string-to-number (match-string 1 switches)) 1 )))
>
> There is a spurious white space after the last 1.
> > +                       ((string-match "+n *\\([0-9]+\\)" switches) (cons 
> > 'continued (- (string-to-number (match-string 1 switches)) 1 )))
>
> Ditto.
> > +                       ((string-match "+n" switches) (cons 'continued 0))
> > +                       ))
>
> These parens should be moved at the end of the previous line.
Nice catch. Before I resubmit the patch. I'll clean up this and the 
other non-standard coding conventions. I haven't submitted patches to 
emacs code before, so I really do appreciate bringing my attention to 
areas where I fail to follow the coding conventions.


> >                  (preserve-indent
> >                   (and switches (string-match "-i\\>" switches)))
> >                  ;; Should labels be retained in (or stripped from) example
> > @@ -2393,7 +2396,7 @@ Assume point is at the beginning of the block."
> >                     (looking-at
> >                      (concat "^[ \t]*#\\+BEGIN_SRC"
> >                              "\\(?: +\\(\\S-+\\)\\)?"
> > -                            "\\(\\(?: +\\(?:-l 
> > \".*?\"\\|[-+][A-Za-z]\\)\\)+\\)?"
> > +                            "\\(\\(?: +\\(?:-l \".+\"\\|[+-]n 
> > *[[:digit:]]+\\|[+-][[:alpha:]]\\)\\)+\\)?"
>
> I don't think "[-+][[:alpha:]]" is correct here and neither was [-+][A-Za-z].  
> We
> only want to match valid switches: k, r and i. Besides, there is no +k,
> +r or +i. Thus, it should be "-[iIkKrR]".

I agree with this. I'll fix this, as well. I think we actually need 
-[iIkKrRnN]|+[nN]
The first part of the regular expression catches [-+][Nn] *[0-9]+ but 
not [-+][Nn] in isolation (which obviously we still need).

> >                              "\\(.*\\)[ \t]*$"))
> >                     (org-match-string-no-properties 1)))
> >                  ;; Get switches.
> > @@ -2403,8 +2406,11 @@ Assume point is at the beginning of the block."
> >                  ;; Switches analysis
> >                  (number-lines
> >                   (cond ((not switches) nil)
> > -                       ((string-match "-n\\>" switches) 'new)
> > -                       ((string-match "+n\\>" switches) 'continued)))
> > +                       ((string-match "-n *\\([0-9]+\\)" switches) (cons 'new 
> > (- (string-to-number (match-string 1 switches)) 1 )))
> > +                       ((string-match "+n *\\([0-9]+\\)" switches) (cons 
> > 'continued (- (string-to-number (match-string 1 switches)) 1 )))
> > +                       ((string-match "-n" switches) (cons 'new 0))
> > +                       ((string-match "+n" switches) (cons 'continued 0))
> > +                       ))
>
> See above.
I'll do some testing and tinkering. The "problem" is that -n  is equal 
to -n 1, and not -n 0.

The cons cell actually holds ('type . (- starting_number 1)). This may 
not be ideal, but seemed the simplest.  There's probably a very simple 
lisp expression that can reduce the cond back to a 3 conditional clause 
to get what I need. (If "see above" was in regards to the regular 
expressions comment).

Unless you were only talking about the extra space, then that's a really 
easy fix. ;)

> > diff --git a/lisp/ox.el b/lisp/ox.el
> > index 5ad17ec..c05f55e 100644
> > --- a/lisp/ox.el
> > +++ b/lisp/ox.el
> > @@ -4148,9 +4148,9 @@ error if no block contains REF."
> >               (when (re-search-backward ref-re nil t)
> >                 (cond
> >                  ((org-element-property :use-labels el) ref)
> > -                ((eq (org-element-property :number-lines el) 'continued)
> > +                ((eq (car (org-element-property :number-lines el)) 'continued)
> >                   (+ (org-export-get-loc el info) (line-number-at-pos)))
> > -                (t (line-number-at-pos)))))))
> > +                (t (+ (or (cdr (org-element-property :number-lines el)) 0) 
> > (line-number-at-pos))))))))
>
> Here, the last two branches of the cond could be replaced with a single
> one calling the new `org-export-get-loc'.
Okay.

> WDYT?
Very Helpful. Thanks for the direction. I'll make the changes/re-write 
to fix these issues and re-submit a patch. Because this goes past (or 
might go past) the 15-line FSF limitation for a "patch." I've got the 
paperwork from the FSF for copyright assignment and will be submitting 
this next week. (Even if a patch would fit into the limit, having the 
paperwork on file won't hurt).


Thanks,
;-b