Skip to content
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

fix: Fix the issue with ineffective --build.full_bin command line argument #748

Merged
merged 4 commits into from
Mar 5, 2025

Conversation

sdfwds4
Copy link
Contributor

@sdfwds4 sdfwds4 commented Mar 3, 2025

No description provided.

Copy link

codecov bot commented Mar 3, 2025

Codecov Report

Attention: Patch coverage is 90.90909% with 1 line in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
main.go 0.00% 1 Missing ⚠️
Files with missing lines Coverage Δ
runner/config.go 76.85% <100.00%> (+0.38%) ⬆️
runner/engine.go 60.03% <100.00%> (+0.07%) ⬆️
main.go 0.00% <0.00%> (ø)

... and 1 file with indirect coverage changes

runner/config.go Outdated
@@ -394,4 +394,12 @@ func (c *Config) WithArgs(args map[string]TomlInfo) {
setValue2Struct(v, value.fieldPath, *value.Value)
}
}

if len(c.Build.FullBin) > 0 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

logic duplicated

air/runner/config.go

Lines 324 to 331 in ad8fbb7

c.Build.ExcludeDir = ed
if len(c.Build.FullBin) > 0 {
c.Build.Bin = c.Build.FullBin
return err
}
// Fix windows CMD processor
// CMD will not recognize relative path like ./tmp/server
c.Build.Bin, err = filepath.Abs(c.Build.Bin)

@xiantang
Copy link
Collaborator

xiantang commented Mar 3, 2025

can we just extract

air/runner/config.go

Lines 324 to 331 in ad8fbb7

c.Build.ExcludeDir = ed
if len(c.Build.FullBin) > 0 {
c.Build.Bin = c.Build.FullBin
return err
}
// Fix windows CMD processor
// CMD will not recognize relative path like ./tmp/server
c.Build.Bin, err = filepath.Abs(c.Build.Bin)
as a function add put after

air/main.go

Line 97 in ad8fbb7

cfg.WithArgs(cmdArgs)

@sdfwds4
Copy link
Contributor Author

sdfwds4 commented Mar 4, 2025

func (c *Config) PostProcess() error {
	if len(c.Build.FullBin) > 0 {
		c.Build.Bin = c.Build.FullBin
		return nil
	}
	// Fix windows CMD processor
	// CMD will not recognize relative path like ./tmp/server
	var err error
	c.Build.Bin, err = filepath.Abs(c.Build.Bin)
	return err
}
cfg.WithArgs(cmdArgs) 
cfg.PostProcess()          // 给cfg增加额外的对外接口,增加使用复杂度

假如我像上面那样修改,PostProcess的功能仅是对c.Build.FullBin和filepath.Abs的简单封装,会给cfg增加额外的对外接口,增加使用复杂度,就不符合KISS原则了

@xiantang xiantang changed the title fix: 修复命令行参数-build.full_bin无效的问题 fix: Fix the issue with ineffective --build.full_bin command line argument Mar 5, 2025
@xiantang
Copy link
Collaborator

xiantang commented Mar 5, 2025

func (c *Config) PostProcess() error {
	if len(c.Build.FullBin) > 0 {
		c.Build.Bin = c.Build.FullBin
		return nil
	}
	// Fix windows CMD processor
	// CMD will not recognize relative path like ./tmp/server
	var err error
	c.Build.Bin, err = filepath.Abs(c.Build.Bin)
	return err
}
cfg.WithArgs(cmdArgs) 
cfg.PostProcess()          // 给cfg增加额外的对外接口,增加使用复杂度

假如我像上面那样修改,PostProcess的功能仅是对c.Build.FullBin和filepath.Abs的简单封装,会给cfg增加额外的对外接口,增加使用复杂度,就不符合KISS原则了

sorry i need to write in English, cause there are contributors all around the world. and i made WithArgs private, and hide logic in preprocess function. i think this way should be better.

@xiantang xiantang merged commit 0448782 into air-verse:master Mar 5, 2025
8 checks passed
caleb-fringer pushed a commit to caleb-fringer/air that referenced this pull request Mar 5, 2025
…ument (air-verse#748)

* Update config.go

* Update flag_test.go

* fix: handle preprocess correctly in command line arguments

* fix: update withArgs documentation to reflect its private status

---------

Co-authored-by: xiantang <zhujingdi1998@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants