Skip to content

THRIFT-6011: Make compiled Go code formatting compatible with gofmt #3493

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
kpumuk merged 1 commit into from
May 19, 2026
+464 −162
Merged

THRIFT-6011: Make compiled Go code formatting compatible with gofmt #3493

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 44 additions & 3 deletions .github/workflows/sca.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,12 +202,53 @@ jobs:
with:
go-version: ${{ matrix.go }}

- name: Run gofmt
- name: Install dependencies
run: |
sudo apt-get update -yq
# shellcheck disable=SC2086
sudo apt-get install -y --no-install-recommends g++ $BUILD_DEPS

- name: Configure
run: |
./bootstrap.sh
# shellcheck disable=SC2086
./configure $(echo $CONFIG_ARGS_FOR_SCA | sed 's/without-go/with-go/')

- uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1
with:
name: thrift-compiler
path: compiler/cpp

- name: Run gofmt on checked-in Go files
# Args:
# -s: Simplify code
# -d: Display changes, and exit non-zero when changes present
# -l: List files that would change
# -e: Report all errors instead of only the first 10
run: gofmt -s -d -e $(git ls-files | grep "\.go$")
run: |
git ls-files '*.go' > /tmp/go-files.txt
xargs -r gofmt -s -l -e < /tmp/go-files.txt > /tmp/gofmt-files.txt
Copy link
Copy Markdown
Member

@fishy fishy May 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with xargs should we use >> over >? with >, if multiple go files have issues, we would only report (cat on the next line) issues from the last go file?

Copy link
Copy Markdown
Member Author

@kpumuk kpumuk May 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, not really. The redirection applies to the xargs command, not to each subprocess it starts. All output from the invoked gofmt processes is collected through that single stdout stream and written to /tmp/gofmt-files.txt.

Copy link
Copy Markdown
Member Author

@kpumuk kpumuk May 19, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ha, an example just came through https://github.com/apache/thrift/actions/runs/26098116881/job/76747159994?pr=3495

cat /tmp/gofmt-files.txt
test ! -s /tmp/gofmt-files.txt

- name: Run gofmt on generated Go files
run: |
chmod a+x compiler/cpp/thrift
thrift_compiler="$PWD/compiler/cpp/thrift"

make -C lib/go/test THRIFT="$thrift_compiler" gopath
make -C test/go THRIFT="$thrift_compiler" gopath
make -C tutorial/go THRIFT="$thrift_compiler" all-local

{
find lib/go/test/gopath/src \
-path lib/go/test/gopath/src/dontexportrwtest -prune -o \
-name '*.go' -type f -print
find test/go/src/gen tutorial/go/gen-go -name '*.go' -type f -print
} > /tmp/generated-go-files.txt

xargs -r gofmt -l -e < /tmp/generated-go-files.txt > /tmp/generated-gofmt-files.txt
Copy link
Copy Markdown
Member

@fishy fishy May 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same comments here.

cat /tmp/generated-gofmt-files.txt
test ! -s /tmp/generated-gofmt-files.txt

lib-python:
needs: compiler
Expand Down
40 changes: 20 additions & 20 deletions compiler/cpp/src/thrift/generate/go_validator_generator.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,11 +239,11 @@ void go_validator_generator::generate_enum_field_validator(std::ostream& out,
}
} else if (key == "vt.defined_only") {
if (values[0]->get_bool()) {
out << indent() << "if (" << context.tgt << ").String() == \"<UNSET>\" ";
out << indent() << "if (" << context.tgt << ").String() == \"<UNSET>\"";
} else {
continue;
}
out << "{" << '\n';
out << " {" << '\n';
indent_up();
out << indent()
<< "return thrift.NewValidationException(thrift.VALIDATION_FAILED, \"" + key + "\", \""
Expand Down Expand Up @@ -276,7 +276,7 @@ void go_validator_generator::generate_bool_field_validator(std::ostream& out,
}
}
}
out << "{" << '\n';
out << " {" << '\n';
indent_up();
out << indent()
<< "return thrift.NewValidationException(thrift.VALIDATION_FAILED, \"" + key + "\", \""
Expand Down Expand Up @@ -308,7 +308,7 @@ void go_validator_generator::generate_double_field_validator(std::ostream& out,
} else {
out << values[0]->get_double();
}
out << "{" << '\n';
out << " {" << '\n';
indent_up();
out << indent()
<< "return thrift.NewValidationException(thrift.VALIDATION_FAILED, \"" + key + "\", \""
Expand Down Expand Up @@ -354,7 +354,7 @@ void go_validator_generator::generate_double_field_validator(std::ostream& out,
} else {
out << values[0]->get_double();
}
out << "{" << '\n';
out << " {" << '\n';
}

indent_up();
Expand Down Expand Up @@ -390,7 +390,7 @@ void go_validator_generator::generate_double_field_validator(std::ostream& out,
} else {
out << values[0]->get_double();
}
out << "{" << '\n';
out << " {" << '\n';
}
indent_up();
out << indent()
Expand Down Expand Up @@ -460,7 +460,7 @@ void go_validator_generator::generate_integer_field_validator(std::ostream& out,
} else {
out << values[0]->get_int();
}
out << "{" << '\n';
out << " {" << '\n';
indent_up();
out << indent()
<< "return thrift.NewValidationException(thrift.VALIDATION_FAILED, \"" + key + "\", \""
Expand Down Expand Up @@ -531,7 +531,7 @@ void go_validator_generator::generate_integer_field_validator(std::ostream& out,
} else {
out << values[0]->get_int();
}
out << "{" << '\n';
out << " {" << '\n';
}
indent_up();
out << indent()
Expand Down Expand Up @@ -608,7 +608,7 @@ void go_validator_generator::generate_integer_field_validator(std::ostream& out,
} else {
out << values[0]->get_int();
}
out << "{" << '\n';
out << " {" << '\n';
}
indent_up();
out << indent()
Expand Down Expand Up @@ -679,17 +679,17 @@ void go_validator_generator::generate_string_field_validator(std::ostream& out,
}
out << ")";
} else if (key == "vt.pattern") {
out << indent() << "if ok, _ := regexp.MatchString(" << target << ",";
out << indent() << "if ok, _ := regexp.MatchString(" << target << ", ";
if (values[0]->is_field_reference()) {
out << "string(";
out << get_field_reference_name(values[0]->get_field_reference());
out << ")";
} else {
out << "\"" << values[0]->get_string() << "\"";
}
out << "); ok ";
out << "); ok";
} else if (key == "vt.prefix") {
out << indent() << "if !strings.HasPrefix(" << target << ",";
out << indent() << "if !strings.HasPrefix(" << target << ", ";
if (values[0]->is_field_reference()) {
out << "string(";
out << get_field_reference_name(values[0]->get_field_reference());
Expand All @@ -699,7 +699,7 @@ void go_validator_generator::generate_string_field_validator(std::ostream& out,
}
out << ")";
} else if (key == "vt.suffix") {
out << indent() << "if !strings.HasSuffix(" << target << ",";
out << indent() << "if !strings.HasSuffix(" << target << ", ";
if (values[0]->is_field_reference()) {
out << "string(";
out << get_field_reference_name(values[0]->get_field_reference());
Expand All @@ -709,7 +709,7 @@ void go_validator_generator::generate_string_field_validator(std::ostream& out,
}
out << ")";
} else if (key == "vt.contains") {
out << indent() << "if !strings.Contains(" << target << ",";
out << indent() << "if !strings.Contains(" << target << ", ";
if (values[0]->is_field_reference()) {
out << "string(";
out << get_field_reference_name(values[0]->get_field_reference());
Expand All @@ -719,7 +719,7 @@ void go_validator_generator::generate_string_field_validator(std::ostream& out,
}
out << ")";
} else if (key == "vt.not_contains") {
out << indent() << "if strings.Contains(" << target << ",";
out << indent() << "if strings.Contains(" << target << ", ";
if (values[0]->is_field_reference()) {
out << "string(";
out << get_field_reference_name(values[0]->get_field_reference());
Expand All @@ -729,7 +729,7 @@ void go_validator_generator::generate_string_field_validator(std::ostream& out,
}
out << ")";
}
out << "{" << '\n';
out << " {" << '\n';
indent_up();
out << indent()
<< "return thrift.NewValidationException(thrift.VALIDATION_FAILED, \"" + key + "\", \""
Expand Down Expand Up @@ -764,7 +764,7 @@ void go_validator_generator::generate_list_field_validator(std::ostream& out,
} else {
out << values[0]->get_int();
}
out << "{" << '\n';
out << " {" << '\n';
indent_up();
out << indent()
<< "return thrift.NewValidationException(thrift.VALIDATION_FAILED, \"" + key + "\", \""
Expand All @@ -773,7 +773,7 @@ void go_validator_generator::generate_list_field_validator(std::ostream& out,
indent_down();
out << indent() << "}" << '\n';
} else if (key == "vt.elem") {
out << indent() << "for i := 0; i < len(" << context.tgt << ");i++ {" << '\n';
out << indent() << "for i := 0; i < len(" << context.tgt << "); i++ {" << '\n';
indent_up();
std::string src = GenID("_elem");
out << indent() << src << " := " << context.tgt << "[i]" << '\n';
Expand Down Expand Up @@ -815,7 +815,7 @@ void go_validator_generator::generate_map_field_validator(std::ostream& out,
} else {
out << values[0]->get_int();
}
out << "{" << '\n';
out << " {" << '\n';
indent_up();
out << indent()
<< "return thrift.NewValidationException(thrift.VALIDATION_FAILED, \"" + key + "\", \""
Expand Down Expand Up @@ -891,7 +891,7 @@ void go_validator_generator::generate_struct_field_validator(std::ostream& out,
} else if (values[0]->is_field_reference()) {
out << indent() << "if !";
out << get_field_reference_name(values[0]->get_field_reference());
out << "{" << '\n';
out << " {" << '\n';
indent_up();
out << indent() << "if err := " << context.tgt << ".Validate(); err != nil {" << '\n';
indent_up();
Expand Down
Loading
Loading