1. In the readme, you mention `org-babel-csharp-target-framework', but
there is no such variable in the code. There is
`org-babel-csharp-target-framework'
True. Thanks for noticing. I adapted this (and the rest of the
documentation where I felt an update was due)
2. The value of `org-babel-csharp-target-framework' is hard-coded. Is
this for a reason?
@Stefan: I sincerely apologize for not having answered your comment on
this email thread! I totally missed that (did not receive a notification).
On the matter: This is not for a reason, and you are of course both
right. Doing something more "advanced" then just hardcoding a fixed
version is definitely a good thing. I was actually also planning to move
over the heuristic I implemented for the tests to the actual module. As
of now, we set the the default framework to the latest available one
that can be discovered on the host system.
3. `org-babel--csharp-default-compile-command' and similar private
functions should probably be named as
`org-babel-csharp--default-compile-command'
Absolutely, this was bogus before. Thanks for mentioning. Changed that.
4. Should `org-babel-csharp-additional-project-flags' be a defcustom?
Yes, it is now.
5. "Construct a csproj file from a list of REFS for the target
FRAMEWORK." docstring is a bit confusing.
Fair. I tried to make its intention more clear.
6. In #'(lambda (u), #' is redundant
Removed.
7. (namespace (make-temp-name "obcs"))
Do you really need to check FS
Apparently, I don't need any "fancyness" for this. So the namespace for
all of the source blocks is now "org.babel.autogen".
8. (string-search ".dll" full-ref)
Maybe use `file-name-extension'?
Sure, this is more "speaking" I guess. Done.
9. (project-name (make-temp-name "obcs"))
As the docstring states, `make-temp-name'
Revised this according to your comment.
10. In the tests, (org-test-for-executable org-babel-csharp-compiler)
should go on top of the file
Looks like I understood that wrongly. Moved to the top, thanks for
clarifying.
11. You are using `type-of' a lot.
I got rid of all/most of them.
12. ert--skip-when
Just (skip-when ...)
Done.
13. In `test-ob-csharp/additional-project-flags-are-respected'
I feel that you overdid lambdas. A simple `dolist' loop would do.
That does not win a price for readability, agreed. I just did that for
not having to repeat the (unwind-protect ...), which would be necessary
as the assertions are orthogonal ((should ..) for the "valid" and
(should-not) for the "invalid" case). But yeah, the benefit of that is
at least questionable. Instead of doing some other "fancyness" with
'dolist' or similar, I just split it into two dedicated tests. IMHO,
this makes the matter "cleanest".
On 11.05.25 19:13, Ihor Radchenko wrote:
poverobuosodonati <poverobuosodon...@gmail.com> writes:
I've documented it accordingly and wrote a test for it here:
...
There is now also a "proper" test for
`org-babel-csharp-additional-project-flags` and updated documentation
for the intended use of this customization:
...
Thanks!
Looks like you have addressed all my comments.
Let me do one more iteration through the code:
1. In the readme, you mention `org-babel-csharp-target-framework', but
there is no such variable in the code. There is
`org-babel-csharp-target-framework'
2. The value of `org-babel-csharp-target-framework' is hard-coded. Is
this for a reason? Or maybe you can instead set it automatically, as
you do in the tests. I do not recall you replying to similar ideas
from Stefan Nobis in
https://list.orgmode.org/orgmode/m11pvpt60k....@nobis-it.eu/
3. `org-babel--csharp-default-compile-command' and similar private
functions should probably be named as
`org-babel-csharp--default-compile-command' (-- moved after
org-babel-csharp prefix). This is to emphasize that these functions
are not a part of org-babel core library.
4. Should `org-babel-csharp-additional-project-flags' be a defcustom?
5. "Construct a csproj file from a list of REFS for the target
FRAMEWORK." docstring is a bit confusing. After first reading, I got
an impression that the function will create an actual file on FS, not
return the contents as string.
6. In #'(lambda (u), #' is redundant
7. (namespace (make-temp-name "obcs"))
Do you really need to check FS and call `make-temp-name'? Or do you
just need a unique symbol?
8. (string-search ".dll" full-ref)
Maybe use `file-name-extension'?
9. (project-name (make-temp-name "obcs"))
As the docstring states, `make-temp-name' will only generate truly
unique file name when the prefix is a full path. So, you probably
want (make-temp-name (file-name-concat org-babel-temporary-directory
"obcs"))
10. In the tests, (org-test-for-executable org-babel-csharp-compiler)
should go on top of the file. The tests are not evaluated in the
order they are defined in the file. Instead, the file is evaluated
(with all its top-level defuns and expressions) first, and then the
tests defined that way are evaluated. So, your check for
org-babel-csharp-compiler, if failed, will prevent the whole test
file from be used anyway.
11. You are using `type-of' a lot. I recommend specific predicates
instead, like `consp' or `stringp'. It will be slightly more compact.
12. ert--skip-when
Just (skip-when ...) will work inside `ert-deftest'
13. In `test-ob-csharp/additional-project-flags-are-respected'
I feel that you overdid lambdas. A simple `dolist' loop would do.
