feat: add C# support #4908
feat: add C# support #4908
Conversation
* Add csharp, boilerplate + minimal execution
* Add initial parser
* Frontend + wasm export of the tree sitter parser
* Arg spread and use cache
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to dc05560 in 2 minutes and 16 seconds
More details
- Looked at
2410lines of code in51files - Skipped
1files when reviewing. - Skipped posting
8drafted comments based on config settings.
1. frontend/src/lib/components/HighlightCode.svelte:59
- Draft comment:
C# language support added. Ensure that the corresponding syntax highlighting is correctly configured. - Reason this comment was not posted:
Confidence changes required:20%
The PR adds C# support, and the changes in this file are consistent with that intent. The language 'csharp' is added to the list of supported languages in various places, ensuring that the UI components handle C# scripts correctly. This is a necessary change to integrate C# support into the frontend.
2. frontend/src/lib/components/WorkerGroup.svelte:125
- Draft comment:
C# language support added to default tags. Ensure that the backend supports these tags as expected. - Reason this comment was not posted:
Confidence changes required:20%
The PR adds C# support, and the changes in this file are consistent with that intent. The language 'csharp' is added to the list of supported languages in various places, ensuring that the UI components handle C# scripts correctly. This is a necessary change to integrate C# support into the frontend.
3. frontend/src/lib/components/common/languageIcons/LanguageIcon.svelte:55
- Draft comment:
C# language label added. Ensure that the label is displayed correctly in the UI. - Reason this comment was not posted:
Confidence changes required:20%
The PR adds C# support, and the changes in this file are consistent with that intent. The language 'csharp' is added to the list of supported languages in various places, ensuring that the UI components handle C# scripts correctly. This is a necessary change to integrate C# support into the frontend.
4. frontend/src/lib/editorUtils.ts:84
- Draft comment:
C# file extension mapping added. Ensure that this mapping is used correctly when handling C# files. - Reason this comment was not posted:
Confidence changes required:20%
The PR adds C# support, and the changes in this file are consistent with that intent. The language 'csharp' is added to the list of supported languages in various places, ensuring that the UI components handle C# scripts correctly. This is a necessary change to integrate C# support into the frontend.
5. frontend/src/lib/infer.ts:169
- Draft comment:
C# parser initialization added. Ensure that the parser is correctly initialized and used for C# scripts. - Reason this comment was not posted:
Confidence changes required:20%
The PR adds C# support, and the changes in this file are consistent with that intent. The language 'csharp' is added to the list of supported languages in various places, ensuring that the UI components handle C# scripts correctly. This is a necessary change to integrate C# support into the frontend.
6. frontend/src/lib/script_helpers.ts:820
- Draft comment:
C# initial code template added. Ensure that this template is used correctly when creating new C# scripts. - Reason this comment was not posted:
Confidence changes required:20%
The PR adds C# support, and the changes in this file are consistent with that intent. The language 'csharp' is added to the list of supported languages in various places, ensuring that the UI components handle C# scripts correctly. This is a necessary change to integrate C# support into the frontend.
7. frontend/src/lib/scripts.ts:121
- Draft comment:
C# added to the list of script languages. Ensure that this is reflected in the UI where script languages are displayed. - Reason this comment was not posted:
Confidence changes required:20%
The PR adds C# support, and the changes in this file are consistent with that intent. The language 'csharp' is added to the list of supported languages in various places, ensuring that the UI components handle C# scripts correctly. This is a necessary change to integrate C# support into the frontend.
8. frontend/src/routes/(root)/(logged)/run/[...run]/+page.svelte:733
- Draft comment:
C# tag added to the exclusion list for job tags. Ensure that this is consistent with how other languages are handled. - Reason this comment was not posted:
Confidence changes required:20%
The PR adds C# support, and the changes in this file are consistent with that intent. The language 'csharp' is added to the list of supported languages in various places, ensuring that the UI components handle C# scripts correctly. This is a necessary change to integrate C# support into the frontend.
Workflow ID: wflow_iHDNh0tsIPdVV0AT
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Deploying windmill with
|
| Latest commit: |
9a16e3f
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://aa754c68.windmill.pages.dev |
| Branch Preview URL: | https://win-444-add-support-for-csha.windmill.pages.dev |
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on e66ee4f in 1 minute and 57 seconds
More details
- Looked at
14587lines of code in162files - Skipped
1files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_KwmYt4g6XiDDW3vU
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 4d541d7 in 24 seconds
More details
- Looked at
10lines of code in1files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_CsxJTnOT2iAClWHa
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 6bae455 in 10 minutes and 44 seconds
More details
- Looked at
79lines of code in5files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_qNGkEUKwOiD4I1Uo
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 94f4556 in 28 seconds
More details
- Looked at
33lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. backend/tests/worker.rs:1812
- Draft comment:
The method nameMainis typically used for entry points in C#. Ensure this is intentional and aligns with the expected behavior in the test context. - Reason this comment was not posted:
Confidence changes required:33%
The C# script in the test function uses a method named 'Main', which is a common convention in C# for entry points. However, the method signature and usage seem correct for the context of this test.
Workflow ID: wflow_Ffkvm3u7pSMFrsYY
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 7befa69 in 48 seconds
More details
- Looked at
25lines of code in2files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. docker/DockerfileFullEe:7
- Draft comment:
Consider using a symbolic link instead of copying thedotnetbinary to/usr/bin. This ensures that any dependencies or configurations are correctly resolved.
RUN ln -s /opt/dotnet-sdk/bin/dotnet /usr/bin/dotnet
- Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_E3sONON4LQ5f2jR2
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
docker/DockerfileFull
Outdated
| @@ -3,4 +3,7 @@ FROM ghcr.io/windmill-labs/windmill:dev | |||
| COPY --from=rust:1.80.1 /usr/local/cargo /usr/local/cargo | |||
| COPY --from=rust:1.80.1 /usr/local/rustup /usr/local/rustup | |||
|
|
|||
| COPY --from=bitnami/dotnet-sdk:9.0.101-debian-12-r0 /opt/bitnami/dotnet-sdk /opt/dotnet-sdk | |||
| RUN cp /opt/dotnet-sdk/bin/dotnet /usr/bin/dotnet | |||
There was a problem hiding this comment.
Consider using a symbolic link instead of copying the dotnet binary to /usr/bin. This ensures that any dependencies or configurations are correctly resolved.
| RUN cp /opt/dotnet-sdk/bin/dotnet /usr/bin/dotnet | |
| RUN ln -s /opt/dotnet-sdk/bin/dotnet /usr/bin/dotnet |
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on ac6b937 in 30 seconds
More details
- Looked at
35lines of code in3files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. docker/DockerfileFull:7
- Draft comment:
Usingln -sto create a symbolic link fordotnetis a good practice as it avoids redundancy and ensures that updates to the SDK are reflected without needing to copy the binary again. - Reason this comment was not posted:
Confidence changes required:0%
The change fromcptoln -sis appropriate for creating a symbolic link, which is generally preferred for executables to avoid redundancy and potential issues with updates.
2. docker/DockerfileFullEe:7
- Draft comment:
Usingln -sto create a symbolic link fordotnetis a good practice as it avoids redundancy and ensures that updates to the SDK are reflected without needing to copy the binary again. - Reason this comment was not posted:
Confidence changes required:0%
The change fromcptoln -sis appropriate for creating a symbolic link, which is generally preferred for executables to avoid redundancy and potential issues with updates.
Workflow ID: wflow_Zlnq06utSLE7wn0w
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 5575899 in 10 minutes and 37 seconds
More details
- Looked at
383lines of code in6files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. backend/windmill-worker/src/handle_child.rs:440
- Draft comment:
Theget_mem_peakfunction returns negative values for errors, which can be misleading. Consider usingOption<i32>orResult<i32, Error>to handle errors more explicitly. - Reason this comment was not posted:
Confidence changes required:50%
The functionget_mem_peakis used in multiple places to fetch memory usage, but it returns negative values for errors. This can be misleading and should be handled more explicitly.
2. backend/windmill-worker/src/python_executor.rs:85
- Draft comment:
Theget_mem_peakfunction returns negative values for errors, which can be misleading. Consider usingOption<i32>orResult<i32, Error>to handle errors more explicitly. This applies to its usage inhandle_python_reqsand other functions. - Reason this comment was not posted:
Confidence changes required:50%
Theget_mem_peakfunction is used in multiple places, includinghandle_python_reqs, to fetch memory usage. It returns negative values for errors, which can be misleading. This should be handled more explicitly.
Workflow ID: wflow_s6yZezT7OQQHi9jf
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on f3a4031 in 24 seconds
More details
- Looked at
9lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. Dockerfile:201
- Draft comment:
Remove the unnecessary newline at the end of the file for consistency. - Reason this comment was not posted:
Confidence changes required:10%
The extra newline at the end of the Dockerfile is unnecessary and should be removed for consistency and cleanliness.
Workflow ID: wflow_FYKV7lmnNX8JyAGK
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on e1d3737 in 26 seconds
More details
- Looked at
9lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. Dockerfile:202
- Draft comment:
Remove the unnecessary trailing newline at the end of the file for consistency. - Reason this comment was not posted:
Confidence changes required:10%
The Dockerfile has an unnecessary trailing newline at the end, which is not a major issue but can be cleaned up for consistency.
Workflow ID: wflow_RTmGGj5TLpuVgmm0
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 8a6a879 in 34 seconds
More details
- Looked at
12lines of code in1files - Skipped
1files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. frontend/package.json:145
- Draft comment:
Ensure the version ofwindmill-parser-wasm-csharpis consistent with otherwindmill-parser-wasm-*dependencies for maintainability. - Reason this comment was not posted:
Confidence changes required:50%
The addition of the C# parser dependency seems correct, but it's important to ensure consistency in versioning with other parsers.
Workflow ID: wflow_uNVx070i3uzkftCB
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on a3fad2c in 46 seconds
More details
- Looked at
47lines of code in3files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. docker/DockerfileFull:6
- Draft comment:
TheRUN pip3 install ansiblecommand is duplicated. Consider consolidating it to avoid redundancy. - Reason this comment was not posted:
Confidence changes required:50%
TheRUN pip3 install ansiblecommand is duplicated in both Dockerfiles. It should be consolidated to avoid redundancy.
2. docker/DockerfileFullEe:6
- Draft comment:
TheRUN pip3 install ansiblecommand is duplicated. Consider consolidating it to avoid redundancy. - Reason this comment was not posted:
Confidence changes required:50%
TheRUN pip3 install ansiblecommand is duplicated in both Dockerfiles. It should be consolidated to avoid redundancy.
Workflow ID: wflow_DdYiAgH4pNLslR2h
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 4034431 in 49 seconds
More details
- Looked at
7lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. backend/ee-repo-ref.txt:1
- Draft comment:
The change in this file is not mentioned in the PR description. Ensure this update is intentional and necessary. - Reason this comment was not posted:
Confidence changes required:50%
The fileee-repo-ref.txthas been modified, but it only contains a single line with a hash. This change might be intentional to update a reference, but it's unclear without additional context. However, since the PR description doesn't mention this file, it's worth pointing out.
Workflow ID: wflow_YvDEUyrXxnGJWNvt
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 5d7240e in 26 seconds
More details
- Looked at
23lines of code in2files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. .github/DockerfileBackendTests:56
- Draft comment:
Consider using a specific version of the dotnet-sdk directly in the Dockerfile to ensure consistency and avoid potential issues with image updates. - Reason this comment was not posted:
Comment did not seem useful.
Workflow ID: wflow_3oOC1BWsRje5StJp
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 02dccdb in 50 seconds
More details
- Looked at
262lines of code in7files - Skipped
0files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. backend/windmill-worker/src/csharp_executor.rs:82
- Draft comment:
Consider ensuring that thenuget.configfile is only deleted if it was successfully created and used. This can prevent potential issues if an error occurs between creation and deletion. - Reason this comment was not posted:
Confidence changes required:50%
The code incsharp_executor.rsis handling the creation and deletion of anuget.configfile. However, the deletion process is not atomic and could potentially leave the file in an inconsistent state if an error occurs between creation and deletion. This could be improved by ensuring that the file is only deleted if it was successfully created and used.
2. frontend/src/lib/components/InstanceSetting.svelte:184
- Draft comment:
EnsureSimpleEditorproperly handles edge cases like large inputs or invalid syntax forcodeareafield type. - Reason this comment was not posted:
Confidence changes required:30%
TheInstanceSetting.sveltefile includes a newcodeareafield type for settings. This is a good addition for handling code-like settings, but ensure that theSimpleEditorcomponent is properly handling all edge cases, such as large inputs or invalid syntax.
3. frontend/src/lib/components/instanceSettings.ts:263
- Draft comment:
Ensurenuget_configsetting is properly validated and sanitized to prevent XML injection or other security issues. - Reason this comment was not posted:
Comment did not seem useful.
Workflow ID: wflow_ZxHADa9SvuIr7F4w
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 16712bd in 33 seconds
More details
- Looked at
93lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. backend/tests/worker.rs:1801
- Draft comment:
The C# test is commented out. Ensure this test is completed and uncommented to verify C# support. - Reason this comment was not posted:
Comment did not seem useful.
Workflow ID: wflow_uo9uayNyKDhtmLDp
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 9a16e3f in 59 seconds
More details
- Looked at
225lines of code in9files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_3z7CPE4wjyxoPTn2
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Important
Add comprehensive C# support across frontend and backend, including editor configuration, syntax highlighting, and script execution.
Editor.svelte,EditorBar.svelte, andHighlightCode.sveltefor syntax highlighting and editor configuration.WorkerGroup.svelteandLanguageIcon.svelteto include C# in language options and icons.script_helpers.ts.windmill-parser-csharpwithlib.rsandwasm_libc.rsfor parsing C# scripts.Cargo.tomlandCargo.lockto includetree-sitter-c-sharpand related dependencies.windmill-workerwithcsharp_executor.rsfor handling C# job execution.main.rsandworker.rsto include C# in supported languages.CSharpIcon.sveltefor visual representation.infer.tsandeditorUtils.ts.This description was created by
for 9a16e3f. It will automatically update as commits are pushed.