Skip to content

Refactor: Split compiler/expr-call.go by concern #78

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.

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

Closed
wants to merge 1 commit into from

Conversation

paralin
Copy link
Member

@paralin paralin commented May 31, 2025

I've split the compiler/expr-call.go file into multiple smaller files to improve organization and maintainability. The original file was responsible for various aspects of Go function call expression translation.

I created the following new files:

  • compiler/expr-call-builtin.go: This handles the translation of Go built-in functions (e.g., make, len, append, string, new, etc.) and includes related helper functions.
  • compiler/expr-call-typeconv.go: This manages the translation of various type conversions (e.g., []rune(string), []byte(string), nil argument conversions, and general slice type conversions).
  • compiler/expr-call-protobuf.go: This contains the logic specifically for translating Protobuf method calls.

The main compiler/expr-call.go file has been refactored to delegate these specific responsibilities to the new files. It now primarily handles general function and method calls, including async call detection.

This refactoring enhances code readability and modularity within the compiler package. All existing Go tests pass with these changes.

I've split the `compiler/expr-call.go` file into multiple smaller files to improve organization and maintainability. The original file was responsible for various aspects of Go function call expression translation.

I created the following new files:
- `compiler/expr-call-builtin.go`: This handles the translation of Go built-in functions (e.g., `make`, `len`, `append`, `string`, `new`, etc.) and includes related helper functions.
- `compiler/expr-call-typeconv.go`: This manages the translation of various type conversions (e.g., `[]rune(string)`, `[]byte(string)`, nil argument conversions, and general slice type conversions).
- `compiler/expr-call-protobuf.go`: This contains the logic specifically for translating Protobuf method calls.

The main `compiler/expr-call.go` file has been refactored to delegate these specific responsibilities to the new files. It now primarily handles general function and method calls, including async call detection.

This refactoring enhances code readability and modularity within the compiler package. All existing Go tests pass with these changes.
@Copilot Copilot AI review requested due to automatic review settings May 31, 2025 07:02
@paralin paralin closed this May 31, 2025
@paralin paralin deleted the refactor/split-expr-call branch May 31, 2025 07:03
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the compiler/expr-call.go file by extracting specialized logic into smaller, concern-focused files, improving modularity and readability.

  • Delegated built-in call handling to expr-call-builtin.go (not shown here).
  • Moved type conversion logic to expr-call-typeconv.go.
  • Extracted Protobuf call translations to expr-call-protobuf.go and removed duplication in compiler/protobuf.go.

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

File Description
compliance/compliance.go Switched type checker invocation from tsc to tsgo, added debug logs and executable check.
compiler/protobuf.go Removed embedded Protobuf method-call logic now moved to a dedicated file.
compiler/expr-call-typeconv.go New file implementing Go-to-TS type conversion call expressions.
compiler/expr-call-protobuf.go New file encapsulating Protobuf method-call translation logic.
Comments suppressed due to low confidence (4)

compiler/expr-call-typeconv.go:6

  • [nitpick] Consider removing or cleaning up commented-out import statements to avoid confusion and improve code clarity.
// "go/token" // Not directly used in this file.

compiler/expr-call-typeconv.go:1

  • New type conversion logic in this file lacks corresponding unit tests; consider adding tests to cover edge cases like nil conversions and named slice types.
package compiler

compiler/expr-call-protobuf.go:1

  • New protobuf method call translations are not covered by tests; add unit tests to validate each mapping for MarshalVT, UnmarshalVT, MarshalJSON, and UnmarshalJSON.
package compiler

compliance/compliance.go:728

  • The code calls os.Stat but the os package is not imported, which will cause a compilation error; add import "os".
if _, statErr := os.Stat(tsgoPath); statErr != nil {


// Check if tsgo executable exists from Go's perspective
if _, statErr := os.Stat(tsgoPath); statErr != nil {
t.Logf("TypeCheck: os.Stat error for %s: %v", tsgoPath, statErr)
Copy link
Preview

Copilot AI May 31, 2025

Choose a reason for hiding this comment

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

[nitpick] Logging a missing tsgo executable without failing the test may hide setup issues; consider using t.Fatalf or explicitly failing when os.Stat returns an error.

Suggested change
t.Logf("TypeCheck: os.Stat error for %s: %v", tsgoPath, statErr)
t.Fatalf("TypeCheck: tsgo executable not found at %s: %v", tsgoPath, statErr)

Copilot uses AI. Check for mistakes.

Comment on lines +20 to +21
// Check if this is a protobuf method call
if methodName == "MarshalVT" || methodName == "UnmarshalVT" || methodName == "MarshalJSON" || methodName == "UnmarshalJSON" {
Copy link
Preview

Copilot AI May 31, 2025

Choose a reason for hiding this comment

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

[nitpick] This long conditional can be simplified by using a switch or a lookup set for better readability and easier extension.

Suggested change
// Check if this is a protobuf method call
if methodName == "MarshalVT" || methodName == "UnmarshalVT" || methodName == "MarshalJSON" || methodName == "UnmarshalJSON" {
// Define a lookup set for protobuf method names
protobufMethods := map[string]struct{}{
"MarshalVT": {},
"UnmarshalVT": {},
"MarshalJSON": {},
"UnmarshalJSON": {},
}
// Check if this is a protobuf method call
if _, exists := protobufMethods[methodName]; exists {

Copilot uses AI. Check for mistakes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

1 participant