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. -- 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>
