poverobuosodonati <poverobuosodon...@gmail.com> writes: > I have put my "patch request" in a public repository. Please find it here > https://codeberg.org/buoso/csharp-babel
Thanks! See my initial comments on the code below: > ;; default header arguments for C# > (defvar org-babel-default-header-args:csharp > '((main . :any) > (namespace . :any) > (project . :any) > (class :any) > (references :any) > (usings :any)) > "Csharp specific header arguments.") Looks like some of the new header arguments can only be "yes"/"no". If so, you should reflect this fact in this variable. > (defcustom org-babel-csharp-compiler "dotnet" > "The program to call for compiling a csharp project.") ob-csharp from org-contrib uses mcs here. I am wondering why you use something different. Is there any syntax difference? Something else? (I have no knowledge of C#) > (defcustom org-babel-csharp-target-framework "net7.0" > "The desired target framework to use.") Do I understand correctly that this more or less a syntax version? If yes, we may want to customize it per-src block as a header argument rather than globally. > (defvar org-babel-csharp-nuget-config nil > "Pass a valid nuget config as documented here > https://learn.microsoft.com/en-us/nuget/reference/nuget-config-file. > This is taken as-is. It should be a string in XML-format.") Same here. Why not header argument? > (defvar org-babel-csharp-additinal-projcect-flags nil > "Will be passed in the 'PropertyGroup' defining the project. > This is taken as-is. It should be a string in XML-format.") ... and here. (Also, typos in the variable name) > (defun org-babel--csharp-preprocess-params (params) > "Make sure PARAMS contains a cons-cell for both `:project' and > `:namespace'." > (unless (assoc :project params) > (push `(:project . ,(symbol-name (gensym))) params)) > (unless (assoc :namespace params) > (push `(:namespace . ,(symbol-name (gensym))) params)) > params) I suggest doing (project-name (or (alist-get :project params) (gensym))) (namespace (or (alist-get :namespace params) (gensym))) inside execute function. It will be much simpler. > (defun org-babel--csharp-parse-usings (usings) > (let ((usinglist)) > (setf usinglist (mapconcat #'(lambda (u) (format "using %s;" u)) usings > "\n")) > usinglist)) I would call this "format" rather than "parse". Parsing is usually string->data, not data->stirng. > (defun org-babel-expand-body:csharp (body params ;; processed-params > ) > (let* ((main-p (not (string= (cdr (assq :main params)) "no"))) > (class-tmp (alist-get :class params)) > (class (pcase class-tmp Can simply do (pcase (alist-get ...) ...) > (defun org-babel-execute:csharp (body params) > "Execute a block of Csharp code with org-babel. > This function is called by `org-babel-execute-src-block'" > (message "executing Csharp source code block") > (org-babel--csharp-preprocess-params params) > (let* ((processed-params (org-babel-process-params params)) Calling `org-babel-process-params' should not be necessary. It is called by Org itself, before running org-babel-execute:... functions. > (dir-param (alist-get :dir params)) > (base-dir (file-name-concat (if dir-param > (ensure-directories-exist) What is the purpose of `ensure-directories-exist' here? To create directory specified in :dir? If yes, you do not need to bother - it is controlled by standard :mkdirp header argument. > (file-truename ".")) > project-name)) Why do you create project inside working directory? These files will remain there for every single src block you run, possibly littering that directory. Most of the babel backends that need to create transient files do it in temp dir via `org-babel-temp-*' functions. > (compile-cmd (concat > org-babel-csharp-compiler > " " "build" > " " "--output" > " " (format "%S" bin-dir) > " " (format "%S"(file-truename base-dir)))) Is it expected that every possible csharp compiler take these exact arguments? > (unless (file-exists-p base-dir) > (make-directory base-dir)) Again, babel should take care about this according to :mkdirp. It is not backend's job to create working directory. > ;; nuget restore > (message (format "dotnet restore %S" project-file)) > (org-babel-eval (format "dotnet restore %S" project-file) "") Why hard-coding "dotnet" command here if it is customizeable? > (org-babel-eval compile-cmd "") > (let ((results (unless (eq project-type 'class) > (org-babel-eval run-cmd "")))) So, project-type that is not 'class will not produce any results. May you please explain why it is useful? > (defun org-babel-csharp-var-to-csharp (var) > "Convert an elisp var into a string of csharp source code > specifying a var of the same value." > (format "var %s = %S;" (car var) (cdr var))) Please rename this into org-babel-variable-assignments:csharp and let it accept params argument. -- Ihor Radchenko // yantar92, Org mode maintainer, Learn more about Org mode at <https://orgmode.org/>. Support Org development at <https://liberapay.com/org-mode>, or support my work at <https://liberapay.com/yantar92>
