Re: ox-latex table tabbing support.

[Kyle Meyer <kyle@kyleam.com>]

Ihor Radchenko writes:

> Daniel Fleischer <danflscr@gmail.com> writes:
>
>> Thank you very much for the patch. It was merged to master in 4a0d951c.
>> Also, you were added to the orgmode contributes at
>> https://orgmode.org/worg/contributors.html.
>>
>> Thanks for the contribution.
>
> This commit triggers
>
> In org-latex--align-string-tabbing:
> ox-latex.el:3713:54: Warning: Unused lexical variable `align'
>
> Looking at the code, it does not look like align variable serve any
> purpose. Can someone please double check if the patch is working as
> intended?

Thanks for flagging this, Ihor.  I was just glancing at this commit
(4a0d951c6) due to seeing this warning.  It's doing

  (let ((align ...))
    (setq align ...))

where the align value is returned, so the align binding can be dropped
altogether.

Daniel, in addition to that, there are at least a few other issues with
4a0d951c6 that should be addressed:

 * the first line of the new docstrings should be a complete sentence.

   For example

     Return an appropriate LaTeX alignment string, for the
     latex tabbing environment.

   should be changed to something like

     Return alignment string for LaTeX tabbing environment.

   See (info "(elisp)Documentation Tips")

 * the indentation is off in several places, including the start of the
   docstrings.  Please indent the code with, e.g., indent-region.

 * one of org-table--org-tabbing's parameters is a typo (contenst),
   which it looks like Ihor pointed out in an earlier review

 * comment typo: documets

 * several spots put opening and trailing parentheses on their own line

   That goes against the usual conventions of this repo:

     $ git grep '^ *)' '*.el' | wc -l
     42
     $ git grep '( *$' '*.el' | wc -l
     17

 * rather than doing something like

     (let ((x ""))
       (setq x <something else>)
       ...)

   just do

     (let ((x <something else>))
      ...)

   And consider whether it's worth adding a binding at all rather than
   inlining the code.

   As an extreme case, org-table--org-tabbing does

     (let ((output (format ...)))
       output)

   rather than

     (format ...)