Issue 16 empty envvars by johnlockwood-wf · Pull Request #18 · rakyll/globalconf

johnlockwood-wf · 2016-09-15T23:25:06Z

Previously, If you set an environmental variable that matches an ENVPREFIX_FLAG_NAME to an empty string, it would not overwrite the flag_name in the INI file. This uses os.LookupEnv to overwrite the flag if it is set at all.

Addresses #16

This reverts commit eb480ac.

johnlockwood-wf · 2016-09-15T23:26:51Z

 // If the flags are already set, values are overwritten
 // by the values in the config file. Defaults are not set
 // if the flag is not in the file.
 func (g *GlobalConf) ParseSet(flagSetName string, set *flag.FlagSet) {


Does ParseSet need to exist at all? It is not tested and It looks like Parse will do the work as long as the flagset is registered.

Parse works on the default flag set, ParseSet works on a particular one. I don't know why Parse doesn't use ParseSet though.

I will try to rewrite a bunch of things after this PR. Thanks for pointing them out.

I stared at TestParse_GlobalAndCustom for a while to figure out how parse, which calls ParseAll was processing the custom FlagSet. Register adds the set to the flags map , which already has the default flag.CommandLine in it, and then ParseAll iterates over all the sets and parses them.

Globally registering a flagset is not always what users can do. We should leave the decision to the user. We should maybe remove registration and ParseAll rather than removing the fundamental flagset-scoped ParseSet.

I don't feel safe breaking the API without existing releases with semantic versioning in place, especially since I have code using globalconf in production. I think ParseSet is fine to keep, I was just a little uneasy about making a change to it without existing tests, so was looking to save the work of adding tests. Would you prefer I add tests proving it does what it says in the comment block? Now that I read that, It has a different behavior than what happens in Parse. It will overwrite already set flags.

Would you prefer I add tests proving it does what it says in the comment block?

Yes, please. I don't think this PR is where we should discuss ParseSet. It is beyond the scope. Could you please file another issue if you have more questions?

johnlockwood-wf · 2016-09-15T23:41:59Z

I take, from the failing ci, it that os.LookupEnv doesn't exist in Go 1.2.2.

rakyll · 2016-09-15T23:57:03Z

 	set.VisitAll(func(f *flag.Flag) {
-		val := getEnv(g.EnvPrefix, flagSetName, f.Name)
-		if val != "" {
+		val, exists := getEnv(g.EnvPrefix, flagSetName, f.Name)


The second bool argument is usually called ok.

if val, ok := getEnv(...); ok { set.Set(f.Name, val) return }

rakyll · 2016-09-15T23:57:27Z


-			val := getEnv(g.EnvPrefix, name, f.Name)
-			if val != "" {
+			val, exists := getEnv(g.EnvPrefix, name, f.Name)


Here again. s/exists/ok.

rakyll · 2016-09-15T23:58:18Z

 }

+func TestParse_GlobalWithEmptyValue(t *testing.T) {
+	t.Log("Given the need to test overwriting a flag with an environmental variable set to an empty string")


Don't log this.

Use a comment block to explain.

I picked up this test explaining style from Go In Action. I saw a number of benefits to it such as when you go test -v the output looks like:

=== RUN TestParse_GlobalWithEmptyValue --- PASS: TestParse_GlobalWithEmptyValue (0.00s) globalconf_test.go:100: Given the need to test overwriting a flag with an environmental variable set to an empty string globalconf_test.go:101: Reset the flags and clear the env globalconf_test.go:106: Set the env var for the c flag to an empty string globalconf_test.go:108: Define the c flag and parse the INI file globalconf_test.go:113: Expect flagC to an empty string and not be the value Hello World from the INI file

This helped me see where failures were happening more quickly along with educating me on what the test was doing.

rakyll · 2016-09-15T23:58:25Z

+		resetForTesting("")
+	}
+
+	t.Log("\tSet the env var for the c flag to an empty string")


rakyll · 2016-09-15T23:58:52Z

+		flagC := flag.String("c", "", "")
+
+		parse(t, "./testdata/global.ini", envTestPrefix)
+		t.Log("\t\tExpect flagC to an empty string and not be the value Hello World from the INI file")


rakyll · 2016-09-15T23:59:16Z

+		parse(t, "./testdata/global.ini", envTestPrefix)
+		t.Log("\t\tExpect flagC to an empty string and not be the value Hello World from the INI file")
+		if *flagC != "" {
+			t.Errorf("\t\tflagC found %v, expected an empty string", *flagC)


don't add \t\t. Test log is formatted nicely by the tool already.

rakyll · 2016-09-16T00:00:44Z

+		parse(t, "./testdata/global.ini", envTestPrefix)
+		t.Log("\t\tExpect flagC to an empty string and not be the value Hello World from the INI file")
+		if *flagC != "" {
+			t.Errorf("\t\tflagC found %v, expected an empty string", *flagC)


Let's use the defacto go test formatting here.

if got, want := *flagC, ""; got != want { t.Errorf("got flag = %q, want empty string", got) }

This is nice. By the time I joined the Go projects I'm working on, the testify package was already adopted and I'm used to seeing require.Equal(t, expected, got)

Use defacto go test formatting Move test explaination to comment block

johnlockwood-wf · 2016-09-16T22:25:23Z

It is weird that github does not mark your comments as addressed anymore.

rakyll · 2016-09-18T23:28:01Z

@johnlockwood-wf, is this your first time contributing to a Google project? If so, could you review the Google CLA underneath https://cla.developers.google.com/clas and sign? Thanks!

johnlockwood-wf · 2016-09-19T18:39:03Z

I did the individual CLA for now and am looking into if we have one of those google groups registered that I can add myself to.
I've run tests on this branch merged with the branch from #19 and the tests still pass, so I feel good about this one.

rakyll · 2016-09-19T19:24:46Z

Thanks!

Looking at #19 shortly.

johnlockwood-wf added 5 commits September 15, 2016 14:53

Add a non-passing test for desired change

1a452fc

Include the test prefix

2bb8382

Make Empty String Env overwrite test pass

29d6e46

Remove the empty string caveat in README

eb480ac

Revert "Remove the empty string caveat in README"

5a10b11

This reverts commit eb480ac.

johnlockwood-wf commented Sep 15, 2016

View reviewed changes

Use go1.5 in travis ci

b6a68e3

rakyll reviewed Sep 16, 2016

View reviewed changes

johnlockwood-wf added 2 commits September 15, 2016 21:06

Use standard ok/then

89f8b6e

Use defacto go test formatting Move test explaination to comment block

Correct pluralization

971b5a4

rakyll merged commit 7afbf15 into rakyll:master Sep 19, 2016

johnlockwood-wf deleted the issue-16-empty-envvars branch September 20, 2016 20:08

Conversation

johnlockwood-wf commented Sep 15, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johnlockwood-wf commented Sep 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johnlockwood-wf commented Sep 16, 2016

Uh oh!

rakyll commented Sep 18, 2016

Uh oh!

johnlockwood-wf commented Sep 19, 2016

Uh oh!

rakyll commented Sep 19, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

johnlockwood-wf commented Sep 15, 2016 •

edited

Loading