Re: packages submissions : org-sql-session and org-blog

[Philip Kaludercic <philipk@posteo.net>]

[-- Attachment #1: Type: text/plain, Size: 427 bytes --]

Phil Estival <pe@7d.nz> writes:

> Hello,
>
> Here are two packages that I'd like to submit to ELPA, with
> the advise and approval of emacs-devel.

Just to be sure, you mean GNU ELPA (not NonGNU ELPA), right?

> - ob-sql-session : https://github.com/flintforge/ob-sql-session
>   meant as a replacement for the orphan package ob-sql-mode.
>   It's in decent state. Version 1.

Here are a inline few comments and suggestions:


[-- Attachment #2: Type: text/plain, Size: 8544 bytes --]

diff --git a/ob-sql-session.el b/ob-sql-session.el
index 96c0949..2a67c1c 100644
--- a/ob-sql-session.el
+++ b/ob-sql-session.el
@@ -2,12 +2,13 @@
 
 ;; Copyright (C) 2024 Free Software Foundation, Inc.
 
-;; Author: Phil Estival pe@7d.nz
-;; Package-Requires: ((emacs "27.2")) ((org "9.5"))
+;; Author: Phil Estival <pe@7d.nz>
+;; Maintainer: Phil Estival <pe@7d.nz>
+;; Package-Requires: ((emacs "27.2") (org "9.5"))
 ;; Keywords: literate programming, reproducible research
 ;; URL: http://github.com/flintforge/ob-sql-session
 
-;; This file is NOT part of GNU Emacs.
+;; This file is part of GNU Emacs.
 
 ;; This program is free software; you can redistribute it and/or modify
 ;; it under the terms of the GNU General Public License as published by
@@ -46,7 +47,6 @@
 ;; $ by the associated value.  This does not declare new variables
 ;; (with a \set command for instance) as they would be stateful and
 ;; span over blocks in sessions.
-;;
 
 ;; In session mode, the connexion parameter are required only when
 ;; login, and no longer required for further requests.
@@ -65,8 +65,8 @@
 (defcustom org-babel-default-header-args:sql-session
   '((:engine . "sqlite"))
   "Default header args."
-  :type '(alist :key-type symbol :value-type string)
-  :group 'org-babel
+  :type '(alist :key-type symbol :value-type string) ;adding an :options as described in (elisp) Composite Types is useful!
+  :group 'org-babel			;perhaps create a new sub-group?
   :safe t)
 
 (defconst org-babel-header-args:sql-session
@@ -109,12 +109,10 @@
                             "-P" "footer=off"
                             "-A"
                             ))
-:
 
 (defun org-babel-execute:sql-session (body params)
   "Execute SQL statements in BODY with PARAMS."
-  (let* (
-         (processed-params (org-babel-process-params params))
+  (let* ((processed-params (org-babel-process-params params))
          (session (cdr (assoc :session processed-params)))
          (engine  (cdr (assoc :engine processed-params)))
          (engine  (intern (or engine (user-error "Missing :engine"))))
@@ -128,10 +126,9 @@
     (setq sql-product engine)
 
     ;; Substitute $vars in body with the associated value. (See also s-format).
-    (mapc
-     (lambda(v) (setq body (string-replace
-                       (concat "$"(symbol-name(car v)))(cdr v) body)))
-     vars)
+    (dolist (v vars)
+      (setq body (string-replace
+                  (concat "$"(symbol-name(car v)))(cdr v) body)))
 
     (with-current-buffer (get-buffer-create "*ob-sql-result*")
       (erase-buffer))
@@ -142,6 +139,7 @@
       (process-send-string (current-buffer) (ob-sql-format-query body engine))
       ;; todo: check org-babel-comint-async-register
       (while (not ob-sql-session-command-terminated)
+	;; could there be a race condition here as described in (elisp) Accepting Output?
         (sleep-for 0.03))
       ;; command finished, remove filter
       (set-process-filter (get-buffer-process sql--buffer) nil)
@@ -155,13 +153,13 @@
     ;; get results
     (with-current-buffer (get-buffer-create "*ob-sql-result*")
       (goto-char (point-min))
+      ;; Byte compiler hints: "‘replace-regexp’ is for interactive use only; use ‘re-search-forward’ and ‘replace-match’ instead."!
       (replace-regexp ;; clear the output or prompt and termination
        (sql-get-product-feature engine :ob-sql-session-clear-output) "")
 
       ;; some client can also directly format to tables
       (when (member "table" results)
         ;;(org-table-convert-region (point-min)(point-max) "|")
-      ;;; or
         (goto-char (point-max)) (delete-char -1) ;; last newline
         (beginning-of-line)
         (let ((end (point)))
@@ -180,15 +178,14 @@ is valid even when `sql-interactive-mode' isn't set.  BUFFER can be a buffer
 object or a buffer name.  The buffer must be a live buffer, have a
 running process attached to it, and, if PRODUCT or CONNECTION are
 specified, its `sql-product' or `sql-connection' must match."
-
   (let ((buffer (get-buffer buffer)))
     (and buffer
          (buffer-live-p buffer)
          (let ((proc (get-buffer-process buffer)))
-           (and proc (memq (process-status proc) '(open run )))))))
+           (and proc (memq (process-status proc) '(open run)))))))
 
 
-(defun org-babel-sql-session-connect (engine params session)
+(defun org-babel-sql-session-connect (engine params session) ;please document the arguments!  use checkdoc!
   "Start the SQL client of ENGINE if it has not in a buffer.
 Clear the intermediate buffer from previous output,
 and set the process filter.
@@ -200,15 +197,13 @@ that clearly identifies the connexion from Emacs, to
 *SQL [session]* in order to retrieved a session with its
 name alone, the other parameters in the header args beeing
 no longer needed while the session stays open."
-
-
   (let* ((sql-database  (cdr (assoc :database params)))
          (sql-user      (cdr (assoc :dbuser params)))
          (sql-password  (cdr (assoc :dbpassword params)))
          (sql-server    (cdr (assoc :dbhost params)))
          ;; (sql-port (cdr (assoc :port params))) ;; to concat to the server
-         (buffer-name (format "%s"(if (string= session "none") ""
-																		(format "[%s]" session))))
+         (buffer-name (format "%s" (if (string= session "none") ""
+				     (format "[%s]" session))))
          ;; (buffer-name (format "%s%s://%s%s/%s"
          ;;                      (if (string= session "none") ""
          ;;                        (format "[%s] " session))
@@ -249,7 +244,7 @@ no longer needed while the session stays open."
         ;; (set-process-filter sql-term-proc
         ;;                     #'ob-sql-session-comint-connect-filter)
         ;;(while (not ob-sql-session-connected))
-        (sleep-for 0.06)
+        (sleep-for 0.06)		;The more or less arbitrary value here makes me uncompfortable
         (with-current-buffer (get-buffer ob-sql-buffer) (erase-buffer))
 
         ;; set the redirection filter
@@ -284,9 +279,9 @@ should also be prompted."
 
       ;; store the regexp used to clear output (prompt1|indicator|prompt2)
       (sql-set-product-feature engine :ob-sql-session-clear-output
-															 (concat "\\(" prompt-regexp "\\)"
-																			 "\\|\\(" ob-sql-session--batch-end-indicator "\n\\)"
-																			 (when prompt-cont-regexp (concat "\\|\\(" prompt-cont-regexp "\\)"))))
+			       (concat "\\(" prompt-regexp "\\)"
+				       "\\|\\(" ob-sql-session--batch-end-indicator "\n\\)"
+				       (when prompt-cont-regexp (concat "\\|\\(" prompt-cont-regexp "\\)"))))
 
       ;; Get credentials.
       ;; either all fields are provided
@@ -326,7 +321,7 @@ should also be prompted."
          (let ((process-environment (copy-sequence process-environment))
                (variables (sql-get-product-feature engine :environment)))
            (mapc (lambda (elem)   ; environment variables, evaluated here
-                   (setenv (car elem) (eval(cadr elem))))
+                   (setenv (car elem) (eval (cadr elem))))
                  variables)
            (funcall (sql-get-product-feature engine :sqli-comint-func)
                     engine
@@ -355,8 +350,8 @@ should also be prompted."
 
           ;; no prompt, connexion failed (and process is terminated)
           (goto-char (point-max))
-          (when (not (re-search-backward prompt-regexp 0 t))
-            (user-error "Connexion failed")))
+          (unless (re-search-backward prompt-regexp 0 t)
+            (user-error "Connection failed"))) ;is this a _user_ error?
         ;;(run-hooks 'sql-login-hook) ; don't
         )
       (sql-progress-reporter-done rpt)
@@ -370,7 +365,6 @@ Carefully separate client commands from SQL commands
 Concatenate SQL commands as one line is one way to stop on error.
 Otherwise the entire batch will be emitted no matter what.
 Finnally add the termination command."
-
   (setq sql-product engine)
   (concat
    (let ((commands (split-string str "\n"))
@@ -411,8 +405,7 @@ its message buffer"
 ;;(add-to-list 'org-structure-template-alist '("sql" . "src sql-session-mode")
 ;; or (customize-variable 'org-structure-template-alist)
 
+;; LocalWords: sql org-mode session
 
 (provide 'ob-sql-session)
-
-;; LocalWords: sql org-mode session
 ;;; ob-sql-session.el ends here

[-- Attachment #3: Type: text/plain, Size: 2875 bytes --]


I agree with Adam that the code is... etwas messy ATM.  Taking a moment
to reindent everything and perhaps add a .dir-locals.el file that
enforces a consistent code style would be of long-term use.

> - Org-weblog (that I may rename org-blog):
>   https://gitlab.com/7dnz/org-weblog. Version 0.
>   Written in literate programming, yet by a lisp beginner
>   in that time.
>     The documentation goes along with the program itself and
>   they get both published as a static site.
>   Demo here:  https://7d.nz/Blogging-with-org.html
>   The code is quite long. This paragraph gives the overall idea:
>   https://7d.nz/Blogging-with-org.html#Introduction.
>   Has a lot of space for improvements, options and customization,
>   but only a few for a v1.

Hmm, I don't know if we have any literate programs on ELPA yet.  If you
prefer that style of development, that is fair, but what I would suggest
would be to add a Makefile and extract the elisp at "compile-time", or
in the case of ELPA when the tarball is being built.

Evaluating the expression in org-weblog.el gives me an error

  Debugger entered--Lisp error: (void-function org-weblog-auto-export-mode)

so I don't think that is ready yet.

Nevertheless, skimming through the .org file (which most certainly
should /not/ be the README btw) the code could also use some cleaning
up, at least if the intention is to publish it as a generic package.
The functions don't seem to adhere to a specific conventional namespace,

Occur on "defun" gives me:

--8<---------------cut here---------------start------------->8---
    319:  (defun org-up-heading (level)
    327:  (defun heading-components-at-level (level)
    366:  (defun split-datetime+title (str)
    375:  (defun timestamp-and-title (str)
    387:  (defun org-html-title-only (str)
    462:  (defun replace-chars-in-string (set-1 with-set-2 str)
    476:  (defun replace-chars (set-1 char-2 str)
    514:  (defun sluggify (str)
    540:  (defun sluggif1 (str)
    588:  (defun append-next-numeral (string-list item)
    623:  (defun exporting-article-p (info) (plist-get info :export-article))
    625:  (defun org-blog-headline (headline contents info)
    761:  (defun src-code-block (src-block contents info)
    792:  (defun blog-html-statistics-cookie (statistics-cookie _contents _info)
    833:  (defun org-blog--build-meta-info (info)
    921:  (defun org-blog-inner-template (contents info)
    939:  (defun org-blog-template (contents info)
   1110:  (defun org-blog--build-pre/postamble (type info)
--8<---------------cut here---------------end--------------->8---

I have my doubts as to how much sense it would make to try and convert
this personal code into a proper package, instead of starting over again :/

> Questions and Criticism are welcome,
> along enthusiasm and motivation.
>
> Cheers,
>
> Phil
>
>

-- 
	Philip Kaludercic on siskin