Hello, Utkarsh Singh <utkarsh190...@gmail.com> writes:
> I am attaching my patch which also include my previous suggestion of > including yes-or-no prompt to org-table-import to allow file which don't > have csv, tsv or txt as extension. I suggest to make several patches. Do not try to stuff as many changes as possible in a single one, please. > + When using org-table-import interactively if we failed to guess > separator then we will be left with a user-error message and an > 'unconverted table'. We can make use of 'temp-buffer' to import our > file after successfully conversion. I'm not sure to understand what you mean. > + Conversion part of org-table-convert-region make a distinction between > '(4) (comma separator) and rest of the separator we should either string > version of comma as AND condition or rewrite to simplify it. Ditto. But it can be the object of another patch. Let's concentrate on `org-table-guess-separator' first. > I am willing to do these possible changes but currently waiting for your > review for org-table-guess-separator as there can be more serious bugs > lurking around on my code which I am considering base for these > changes. You should definitely write tests for this function. Here's a start: (ert-deftest test-org-table/guess-separator () "Test `test-org-table/guess-separator'." ;; Test space separator. (should (equal " " (org-test-with-temp-text "a b\nc d" (org-table-guess-separator (point-min) (point-max))))) (should (equal " " (org-test-with-temp-text "a b\nc d" (org-table-guess-separator (point-min) (point-max))))) ;; Test "inverted" region. (should (equal " " (org-test-with-temp-text "a b\nc d" (org-table-guess-separator (point-max) (point-min))))) ;; Do not error on empty region. (should-not (org-test-with-temp-text "" (org-table-guess-separator (point-max) (point-min)))) (should-not (org-test-with-temp-text " \n" (org-table-guess-separator (point-max) (point-min))))) > + (end (save-excursion > + (goto-char (max beg end0)) This should be beg0 instead of beg above. > + (sep-regexp '(("," (rx bol (1+ (not (or ?\n ?,))) eol)) > + ("\t" (rx bol (1+ (not (or ?\n ?\t))) eol)) > + (";" (rx bol (1+ (not (or ?\n ?\;))) eol)) > + (":" (rx bol (1+ (not (or ?\n ?:))) eol)) > + (" " (rx bol (1+ (not (or ?' ?\" )) > + (not (or ?\s ?\;)) > + (not (or ?' ?\"))) eol)))) Use (sep-regexp (list (list "," (rx bol (1+ (not (or ?\n ?,))) eol)) (list "\t" (rx bol (1+ (not (or ?\n ?\t))) eol)) (list ";" (rx bol (1+ (not (or ?\n ?\;))) eol)) (list ":" (rx bol (1+ (not (or ?\n ?:))) eol)) (list " " (rx bol (1+ (not (or ?' ?\" )) (not (or ?\s ?\;)) (not (or ?' ?\"))) eol)))) so you don't need eval below, and rx forms become constants when compiled. > + sep) This `sep' binding can be removed. > + (unless (= beg end) > + (save-excursion > + (goto-char beg) > + (catch :found > + (pcase-dolist (`(,sep ,regexp) sep-regexp) > + (save-excursion > + (unless (re-search-forward (eval regexp) end t) You can drop the `eval'. > (when (and (called-interactively-p 'any) > - (not (string-match-p (rx "." (or "txt" "tsv" "csv") eos) file))) > + (not (string-match-p (rx "." (or "txt" "tsv" "csv") eos) file)) > + (not (yes-or-no-p "File does not havs .txt .txt .csv as > extension. Do you still want to continue? "))) "does not have" and ".txt" -> ".tsv" I guess. Also please provide a patch with a commit message, possibly using `git format-patch'. Thanks! Regards, -- Nicolas Goaziou