Refactor generator validation

[KevinGimbel]

This patch reverts the changes made in #10 and places the generator
validation inside the NewColumns function.
Tests have been adjusted so that they can handle expected error cases for unknown generators.

I manually tested it and ran the tests locally with make test and I believe this time nothing is broken! 😄

[lucapette]

@KevinGimbel this looks great!

One small detail: we changed the test assertion to if actual, err := fakedata.NewColumns(tt.input); !reflect.DeepEqual(actual, tt.expected) && ((err != nil) != tt.wantErr) which makes sense because NewColumns now can return an error. But we don't print the error if the test fail.

I'm thinking maybe we should extract actual, err := fakedata.NewColumns(tt.input) and have an assertion for actual and another for err. It'd make it very explicit we're testing both things.

I can take care of it if you don't have the time. The code looks pretty good already as it is. Thank you very much!

[KevinGimbel]

Thanks for the kind words!

I'm thinking maybe we should extract actual, err := fakedata.NewColumns(tt.input) and have an assertion for actual and another for err. It'd make it very explicit we're testing both things.

That makes sense. Handling both cases would mean changing all Column tests to something like this?

t.Run(tt.name, func(t *testing.T) {
	actual, err := fakedata.NewColumns(tt.input)
	
	// Test fails with unexpected result
	if !reflect.DeepEqual(actual, tt.expected) {
		t.Errorf("NewColumns() = %v, want %v", actual, tt.expected)
	}

	// Dont't want error but got error
	if (err != nil) && !tt.wantErr {
		t.Errorf("NewColumns() = Dont want error but got: %v", err)
	}
})

Printing the "expected" errors from NewColumns would not make much sense or am I missing something? When we have wantErr: true we expect an error from the function, so no need to print it. When we have wantErr: false and get an error, there is something wrong.

I can make the changes in my lunch break today. :)

[lucapette]

@KevinGimbel that looks perfect to me!

[KevinGimbel]

@lucapette yay! 🎉 I'll implemented it later today then.

[KevinGimbel]

I made the changes to all tests in column_test.go so they handle the newly returned error. I had to slightly adjust the check for actual .

if !reflect.DeepEqual(actual, tt.expected) && !tt.wantErr {
    t.Errorf("NewColumns() = %v, want %v", actual, tt.expected)
}

For tests that should fail we need the check for !tt.wantErr, otherwise the tests fail the DeepEqual check as seen below.

    --- FAIL: TestNewColumns/two_columns,_one_column_fails (0.00s)
        column_test.go:28: NewColumns() = [{email email } {domain domain } {unsupportedgenerator unsupportedgenerator }], want []
    --- FAIL: TestNewColumns/one_column,_all_fails (0.00s)
        column_test.go:28: NewColumns() = [{madeupgenerator madeupgenerator }], want []

These two tests are defined with wantErr: true so by checking && !tt.wantErr we can skip the error reporting here.

[lucapette]

@KevinGimbel so cool! thank you again!

[KevinGimbel]

@lucapette you're welcome! It's been fun to work on some "real" Go code. :D if you ever want some more help hit me up.

[lucapette]

@KevinGimbel I have a small list of generators and some little gardening I'd like to do. I'll be opening issues in the coming days. If you'd like to take over one of the issues, I'd very grateful and happy to help!

[KevinGimbel]

@lucapette I'll keep an eye on issues and will let you know when I have the time to work on one or more. It's a good exercise for me, both in Go and general GitHub workflow/contributing to other people's code. Thank you again for being so welcoming!