Re: bug#47885: [PATCH] org-table-import: Make it more smarter for interactive use

[Maxim Nikulin <manikulin@gmail.com>]

On 04/06/2021 11:04, Utkarsh Singh wrote:
> On 2021-06-02, 23:44 +0700, Maxim Nikulin wrote:
> 
>> On 02/06/2021 22:08, Utkarsh Singh wrote:
>>> ;;;###autoload
>>>    (defun org-table-import (file separator)
>>> @@ -955,12 +971,13 @@ lines.  It can have the following values:
>>>    - integer When a number, use that many spaces, or a TAB, as field separator.
>>>    - regexp  When a regular expression, use it to match the separator."
>>>      (interactive (list (read-file-name "Import file: ")
>>> -                     (prefix-numeric-value current-prefix-arg)))
>>> +                     current-prefix-arg))
>>
>> It seems, prefix argument works now. Let me remind that file name
>> completion was working better before your change.

If you are still going to update this patch, I recommend you to check if 
`list' form of `interactive' declaration is supported by Emacs 24.3. 
Recently Kyle had to almost revert one my patch. Fortunately a simpler 
alternative supported by old Emacs versions were already discussed.

>> I have noticed a couple of error messages. Unsure what is going wrong, I
>> hope, command to launch emacs is correct (-Q -L ~/src/org-mode/lisp
>> test.org).
>>
>> At startup:
>> Eager macro-expansion failure: (error "rx ‘not’ syntax error: (or 10 44)")
>>
>> In response to M-x org-table-import:
>> rx-check-not: rx ‘not’ syntax error: (or 10 44)
> 
> Is `rx' library loading correctly? If no then you can try:
> 
> +(require 'rx)

This line should be added to org-table.el, but notice "rx ‘not’ syntax 
error: (or ". This logical operators have a bit different meaning for 
regular expression.

Nicolas suggested you to use `list' to avoid `eval'. Certainly `eval' 
should be avoided whenever possible, but there is alternative with "`" 
and "," instead of "'". Though I often prefer `list'.

> 
> Can you also share test.org so that I can test locally?

I can reproduce it with an empty org file. Emacs-26.3. So I am curious 
if your tested your patch at all.

> Now on the topic of CSV parser, I have succesfully implemented a CSV
> parser in less than 65 LOC which also preserves newline character but I
> am facing a dilemma on how should I represent it as a Org table.  For
> ex:
...
> 6,"Cell with new
> Line",6.28

Due to
https://orgmode.org/worg/org-faq.html#Tables
"Will there ever be support for multiple lines in a table field? No."
(there are may be some tricks in Org manual) any variant is acceptable, 
even current one. Since during export split cell is considered as 
different rows, it is responsibility of the user to preprocess original 
file and import it in a form suitable for Org.

However independently of chosen variant, it would be great to get list 
of warnings with ambiguous lines similar to compiler errors. Such 
feature is not implemented in Org yet, the most close is `org-lint' 
result (its behavior differs from compiler errors).

> 2. Represent every newline as distinct cell.  I was trying to implement
> this method but the algorithm that I came up with requires time
> complexity of O(n^2) to just print each row which is too much for large
> CSV files.
> 
> (defun org-table--print-row (row)
>    (let ((maxlines 1)
>          list1 list2 tmp)
>      ;; get maxlines
>      (dolist (i row)

I expect complexity O(number of lines in result). I do not like 
excessive usage of `dolist'. I would try slicing subrows using `mapcar' 
while there is at least one non-nil element.

P.S. Have you read "Structure and Interpretation of Computer Programs" 
by Abelson, Sussman, and Sussman?
https://sarabander.github.io/sicp/html/index.xhtml
or https://mitpress.mit.edu/sites/default/files/sicp/index.html
While reading, it is necessary to realize that elisp is not scheme, tail 
recursion should be avoided in elisp and there are over differences.