First of all, thank you very much for the detailed review, this is awesome!
I have adapted some bits and pieces and tried to elaborate on the open points in the following.
Looks like some of the new header arguments can only be "yes"/"no". If so, you should reflect this fact in this variable.
I adapted it where applicable such that header arguments take something that is not :any where possible.
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#)
In fact, this was a deliberate decision. First and foremost, the mono c# compiler (mcs) started out back in the days when .NET was not cross-platform. This has changed, and .NET (=dotnet=) is now cross-platform. Mono does not support "new" language features anymore (meanwhile the language evolved to [[https://learn.microsoft.com/en-us/dotnet/csharp/whats-new/csharp-13][version 13]], Mono only supports up to version 7 though). Further, it is recommended by mono itself to migrate to .NET (see [[https://github.com/mono/mono][here]]) after Microsoft bought etc. So all in all, I'd argue that using dotnet over mono is a good idea looking forward.
Actually it is a tad different. This is the runtime version (part of the [[https://dotnet.microsoft.com/en-us/download/dotnet][.NET SDK]]). It kind of defines for which runtime the program/code block is compiled. A loose equivalent comparing it to C++ would be something along the lines of language standard and compiler version. E.g., you need at [[https://en.cppreference.com/w/cpp/compiler_support/17][least GCC version 6]] to get support for the C++17 language features of [[https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2014/n4230.html][nested namespace definitions]]. Similar, you would need [[https://learn.microsoft.com/en-us/dotnet/csharp/whats-new/csharp-13][the .NET9 runtime]] to use the ~ref struct~ from [[https://learn.microsoft.com/en-us/dotnet/csharp/whats-new/csharp-13#allows-ref-struct][language version C# 13]]. I am a bit undecided whether or not it would make sense to expose this as a header argument as I understand this to be rather "static" as in there might not be a desire to change this from code block to code block. What do you think?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.
This is something very specific. It is intended to define NuGet feeds other than https://www.nuget.org/. I would assume that this is not necessary very often. The most significant usecase for sure are "internal" nuget feeds. I.e., such that people use within their organizations in order not to leak IP. Exposing it as a header argument as a raw xml-string might not be good as it would potential be quite a "chatty" string. But rethinking this, it is presumably better to feed in a file path instead of a raw string (in this case a path to a /nuget.config/ file since this is what's ultimately happening with the raw string anyway). And then yeah, why not expose it as a header argument.Same here. Why not header argument?
This might again become "chatty", i.e., a longish string. Further, this is intended only for "completeness" reasons, i.e., I don't really expect that this would be used frequently. What do you think: should it be a header argument nonetheless? In the meantime, I fixed the typo at least ;)... and here. (Also, typos in the variable name)
The "problem" I have here is the following: it is not required to set the namespace in the header arguments, but I need the same value for that key in the =org-babel-expand-body:csharp= and =org-babel-execute:csharp=. Thus, I "preprocess" the =params= and use a copy of that which contains keys for ":project" and ":namespace". Now for "execution only", it would be totally sufficient to do this only in the execute function. For code-block expanding not to fail when there is either no namespace or project key (or both), I do this also in the expand function.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.
I would call this "format" rather than "parse". Parsing is usually string->data, not data->stirng. Can simply do (pcase (alist-get ...) ...)
Makes sense, done!
Calling `org-babel-process-params' should not be necessary. It is called by Org itself, before running org-babel-execute:... functions.
Removed that.
This is a bit involved: yes, the directory passed with :dir is generated automatically. But, the respective project will be within an additional subdirectory that is called like the project itself is called. It is not really obvious by this `ensure-directories-exist' function, but `.' here denotes the path :dir/:project and :dir does not contain a :project directory unless we ensure it.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.
This was "stolen" from ob-java.el, which does it like this. But yes, nothing holds us back from using org-babel-temporary-directory. Adapted it accordingly.Why do you create project inside working directory? These files will remain there for every single src block you run, possibly littering that directory.
You're right, there might be different needs for compilation. I've made it customizable with =org-babel-csharp-generate-compile-command=Is it expected that every possible csharp compiler take these exact arguments?
This follows the same rational as above: we must ensure that directory with name ":project" is existing before writing the generated files there.Again, babel should take care about this according to :mkdirp. It is not backend's job to create working directory.
That is a good point! I have made this customizable as well in =org-babel-csharp-generate-restore-command=Why hard-coding "dotnet" command here if it is customizeable?
The other way round actually. When the project type is 'class, we do not run it as there is no executable present in such a case. We want that when the code-block is intended to be a "class library", i.e., does not have a main function and therefore no executable entrypoint. This is used to promote a code-block to the status of a "shared library" that can be included in other code blocks/csharp projects to make use of the functionality it provides.So, project-type that is not 'class will not produce any results. May you please explain why it is useful?
Can you help me out here understanding this better? Is it a convention or does it mean "refactoring" or doing it differently?Please rename this into org-babel-variable-assignments:csharp and let it accept params argument.
On 04.02.25 20:19, Ihor Radchenko wrote:
poverobuosodonati <poverobuosodon...@gmail.com> writes:I have put my "patch request" in a public repository. Please find it here https://codeberg.org/buoso/csharp-babelThanks! 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-tmpCan 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.
