Tim Hansinger <[email protected]> writes: > I mainly worked on my own till now (when it comes to coding). Just > recently I started to work with github/commits/PRs.
Wow! You are advancing fast! > It is always good to learn something new. Please have a look at the > attached patch. I created it according to your info using Magit. The patch applies cleanly on `main'. Way to go! I did a second review on your code below. When your patch is reviewed, you fix things up, and then send a new patch. In Magit, this is simple: you change some files, stage as normal with `S', and then `c e' (commit, extend) your commit. If the commit message needs editing, you do `r w' (rebase, reword) on the commit. When all is done, you create a new patch file from your commit, which you already know how to do. :) > This time this single patch contains both changes (the one to > ob-plantuml.el and the one to the test-ob-plantuml.el). Very nice! >> Rudolf Adamkovič [email protected] [he/him] >> http://adamkovic.org > From 7b727afbb9fe118909752c62608e0f9d247c7bad Mon Sep 17 00:00:00 2001 > From: Tim Hansinger <[email protected]> > Date: Mon, 15 Sep 2025 17:52:15 +0200 > Subject: [PATCH] ob-plantuml.el: Fix for var definition ^^^^^^^^^^^^^^^^^^^^^^ I think we should give this commit a better description. Firstly, always use active voice, e.g. "Fix..." is correct, "Fix for..." or "Fixing..." or "Fixed..." are not. This is standard in the world of Git, FYI. Secondly, the description is a bit unclear, perhaps too low-level. Is "var" a Lisp variable or a Babel variable? And, importantly, does the patch really fix its "definition"? Think about how you would explain this to a user, in active voice, e.g. "Make Babel variables work inside of @start/@end". > + ((lines (split-string body "\n")) (first-line (car lines)) ^^^^^^^^^^^^^^^^^^^^^^^^ This should be on its own line, like all the other forms in the containing `let*'. [In fact `let*' does not need a new-line after it either.] (let* ((lines (split-string body "\n")) (first-line (car lines)) (rest-lines (cdr lines)) (assignment-lines (mapconcat 'identity assignments "\n")) (clean-first-line (replace-regexp-in-string "\\\\@" "@" first-line))) ...) > (assignment-lines (mapconcat 'identity assignments "\n")) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ You can use `string-join'. Consider: (mapconcat 'identity '("A" "B") "x") ;; => "AxB" (string-join '("A" "B") "x") ;; => "AxB" > + (clean-first-line > + (replace-regexp-in-string "\\\\@" "@" first-line))) > + (concat clean-first-line "\n" assignment-lines > + (when rest-lines > (concat "\n" (mapconcat 'identity rest-lines "\n"))))) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Ditto. :) Rudy -- "'Contrariwise,' continued Tweedledee, 'if it was so, it might be; and if it were so, it would be; but as it isn't, it ain't. That's logic.'" --- Lewis Carroll, Through the Looking Glass, 1871/1872 Rudolf Adamkovič <[email protected]> [he/him] http://adamkovic.org
