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 for empty default value #66

Merged
merged 1 commit into from
Mar 22, 2021
Merged

fix for empty default value #66

merged 1 commit into from
Mar 22, 2021

Conversation

wweir
Copy link
Contributor

@wweir wweir commented Mar 19, 2021

No description provided.

@codecov
Copy link

codecov bot commented Mar 19, 2021

Codecov Report

Merging #66 (3deab44) into main (32a7c36) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #66      +/-   ##
==========================================
+ Coverage   92.22%   92.24%   +0.02%     
==========================================
  Files           3        3              
  Lines         360      361       +1     
==========================================
+ Hits          332      333       +1     
  Misses         23       23              
  Partials        5        5              
Impacted Files Coverage Δ
aconfig.go 92.37% <100.00%> (+0.06%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 32a7c36...3deab44. Read the comment docs.

@wweir
Copy link
Contributor Author

wweir commented Mar 19, 2021

The field AllFieldRequired will not work in the example code

	type MyConfig struct {
		Port int `usage:"just give a number"`
		Auth struct {
			User string `default:"def-user"`
			Pass string `default:"def-pass"`
		}
		Pass string `flag:"sec_ret" required:"true"`
	}

	var cfg MyConfig
	loader := aconfig.LoaderFor(&cfg, aconfig.Config{
		AllFieldRequired: true,
		// SkipDefaults:     true,
	})

	if err := loader.Load(); err != nil {
		panic(err)
	}

	log.Println(cfg)

@cristaloleg
Copy link
Member

Hi, I don't get what you're expecting. Is this related to this line? https://github.com/cristalhq/aconfig/blob/main/aconfig.go#L48

@wweir
Copy link
Contributor Author

wweir commented Mar 22, 2021

@cristaloleg Yes, the option AllFieldRequired and the tag required dose not work correctly.
It will be initialized by the empty default value.

@wweir
Copy link
Contributor Author

wweir commented Mar 22, 2021

The code in PR is not the best way to fix it.

@cristaloleg
Copy link
Member

I don't mean it's incorrect, I'm asking is this related to the AllFieldRequired documented behaviour ?

@wweir
Copy link
Contributor Author

wweir commented Mar 22, 2021

Yes, I have already answered.
The flag AllFieldRequired does not work correctly if SkipDefaults is false.

Have you do any testing on the behavior of AllFieldRequired ?

@cristaloleg
Copy link
Member

Now it's better, you haven't mentioned SkipDefaults before.

Copy link
Member

@cristaloleg cristaloleg left a comment

Choose a reason for hiding this comment

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

lgtm

@cristaloleg cristaloleg merged commit 531db3d into cristalhq:main Mar 22, 2021
@cristaloleg
Copy link
Member

Thanks 🎉

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