Port vexillographer to python #12551
Conversation
|
This is cool! Just one question -- No offensive, just my personal feeling, it looks like AI is involved in programming, not quite sure about the policy here though, but in case (again, it is just my feeling, I have no objection on AI) if it is being used, it might be nice to mention which AI it is using, so people who knows the policy may have more information? If no AI is involved, please ignore me. |
Result of foundationdb-pr-clang-ide on Linux RHEL 9
|
|
@xis19 thanks for flagging, should have mentioned in the PR description. The initial porting work was done using Anthropic Sonnet 4.5. If the code is too AI sloppy (overly verbose comments and boilerplate) to be considered for merging I'm happy to rework it further. |
Result of foundationdb-pr-clang-arm on Linux CentOS 7
|
I'v no preference, I mean, if it works it is good. Yet I heard there was AI related rule but you know, never actually remember what it was... @jzhou77 may provide you more information. |
Result of foundationdb-pr-cluster-tests on Linux RHEL 9
|
Result of foundationdb-pr-clang on Linux RHEL 9
|
Result of foundationdb-pr on Linux RHEL 9
|
I think it's fine. We are allowed to use AI tools for this project. |
xis19
left a comment
xis19
left a comment
There was a problem hiding this comment.
Some minor improvements can be good.
| Args: | ||
| output_path: Path where output file(s) should be written | ||
| """ | ||
| pass |
There was a problem hiding this comment.
I think here raise NotImplementedError() is a more proper response for abstract methods
| * | ||
| * This source file is part of the FoundationDB open source project | ||
| * | ||
| * Copyright 2013-2024 Apple Inc. and the FoundationDB project authors |
There was a problem hiding this comment.
It will be more helpful to interpolate the copyright line using datetime library.
| # | ||
| # This source file is part of the FoundationDB open source project | ||
| # | ||
| # Copyright 2013-2024 Apple Inc. and the FoundationDB project authors |
| template = self.env.get_template(template_name) | ||
| return template.render(**context) | ||
|
|
||
| def write_file(self, path: str, content: str) -> None: |
There was a problem hiding this comment.
Not a big deal if ignored, but it is better to use pathlib.Path in this case.
| return f"({option.param_type.value}) {option.param_desc or ''}" | ||
|
|
||
| @abstractmethod | ||
| def write_files(self, output_path: str) -> None: |
| @@ -0,0 +1,12 @@ | |||
| #include "fdbclient/FDBOptions.g.h" | |||
|
|
|||
| @@ -0,0 +1,36 @@ | |||
| package com.apple.foundationdb; | |||
|
|
|||
| @@ -0,0 +1,86 @@ | |||
| package com.apple.foundationdb; | |||
|
|
|||
| @@ -0,0 +1,33 @@ | |||
| # FoundationDB Python API | |||
There was a problem hiding this comment.
generate the license from code?
| # all copies or substantial portions of the Software. | ||
|
|
||
| # THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR | ||
| # IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, |
There was a problem hiding this comment.
In Ruby, multiple line comments can be grouped by
=begin
COMMENT
=end
so leading # can be removed
4 participants
This PR ports Vexillographer codegen from C# to Python. The aim is to piece-by-piece replace all dependencies on Mono/Dotnet to reduce the build system SBOM. Python is required as part of the bindings build process anyway and a much more minimal / usual build toolchain component. Given jinja2 is already a dependency we use it here as well.
The change was tested by comparing outputs of codegen for all languages between the reimplementation and original codebase.
All outputs are binary-equivalent, except ruby (one missing newline) and Java (usage of \n instead of a mix of \n & \r).
This is the first of 4 changes replacing CS components with Python reimplementations. Next up are traceloghelper, coveragetool and actorcompiler before the Mono dependency can be dropped.
Code-Reviewer Section
The general pull request guidelines can be found here.
Please check each of the following things and check all boxes before accepting a PR.
For Release-Branches
If this PR is made against a release-branch, please also check the following:
release-branchormainif this is the youngest branch)