Copilot commented on code in PR #871:
URL:
https://github.com/apache/incubator-seata-go/pull/871#discussion_r2312331465
##########
pkg/saga/statemachine/engine/config/default_statemachine_config.go:
##########
@@ -364,28 +359,27 @@ func (c *DefaultStateMachineConfig) LoadConfig(configPath
string) error {
return fmt.Errorf("failed to read config file: path=%s,
error=%w", configPath, err)
}
- parser := parser.NewStateMachineConfigParser()
- smo, err := parser.Parse(content)
- if err != nil {
- return fmt.Errorf("failed to parse state machine definition:
path=%s, error=%w", configPath, err)
- }
-
var configFileParams ConfigFileParams
- if err := json.Unmarshal(content, &configFileParams); err != nil {
+ ext := strings.ToLower(filepath.Ext(configPath))
+
+ switch ext {
+ case ".json":
+ if err := json.Unmarshal(content, &configFileParams); err !=
nil {
+ return fmt.Errorf("failed to unmarshal config file as
JSON: %w", err)
+ }
+ case ".yaml", ".yml":
if err := yaml.Unmarshal(content, &configFileParams); err !=
nil {
return fmt.Errorf("failed to unmarshal config file as
YAML: %w", err)
- } else {
- c.applyConfigFileParams(&configFileParams)
}
- } else {
- c.applyConfigFileParams(&configFileParams)
- }
-
- if _, exists := c.stateMachineDefs[smo.Name]; exists {
- return fmt.Errorf("state machine definition with name %s
already exists", smo.Name)
+ default:
+ if err := json.Unmarshal(content, &configFileParams); err !=
nil {
+ if err := yaml.Unmarshal(content, &configFileParams);
err != nil {
+ return fmt.Errorf("failed to unmarshal config
file (unknown type): %w", err)
Review Comment:
[nitpick] The fallback logic attempts JSON first, then YAML for unknown file
extensions. This could mask JSON parsing errors when the file is actually
intended to be YAML. Consider requiring explicit file extensions or providing a
more explicit error message that indicates both formats were attempted.
```suggestion
jsonErr := json.Unmarshal(content, &configFileParams)
if jsonErr != nil {
yamlErr := yaml.Unmarshal(content, &configFileParams)
if yamlErr != nil {
return fmt.Errorf(
"failed to unmarshal config file
(unknown type): attempted JSON (error: %v), then YAML (error: %v)",
jsonErr, yamlErr,
)
```
##########
.licenserc.yaml:
##########
@@ -80,7 +80,7 @@ header: # `header` section is configurations for source codes
license header.
- 'VERSION'
- ".errcheck-exclude"
- ".golangci.yml"
- - '.pre-commit-config.yaml'
+ - '.pre-commit-saga_config.yaml'
Review Comment:
The filename '.pre-commit-saga_config.yaml' appears to be incorrect. Based
on the context, this should likely be '.pre-commit-config.yaml' (the standard
pre-commit configuration filename).
```suggestion
- '.pre-commit-config.yaml'
```
##########
pkg/saga/statemachine/engine/config/default_statemachine_config.go:
##########
@@ -648,13 +653,11 @@ func NewDefaultStateMachineConfig(opts ...Option)
*DefaultStateMachineConfig {
opt(c)
}
- if err := c.LoadConfig("config.yaml"); err == nil {
- log.Printf("Successfully loaded config from config.yaml")
- } else {
- log.Printf("Failed to load config file (using default/env
values): %v", err)
+ if err := c.Init(); err != nil {
+ return nil, fmt.Errorf("failed to initialize state machine
config: %w", err)
}
Review Comment:
[nitpick] The constructor now calls Init() automatically, which could cause
issues if callers need to configure additional options after construction but
before initialization. Consider separating construction from initialization to
give callers more control over the initialization timing.
```suggestion
// Initialization is now the caller's responsibility.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@seata.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscr...@seata.apache.org
For additional commands, e-mail: notifications-h...@seata.apache.org
