Custom SQL column names are broken #12

Closed
kevingimbel opened this Issue May 15, 2017 · 10 comments

Comments

2 participants
@kevingimbel
Collaborator

kevingimbel commented May 15, 2017

I didn't know about the custom sql column names when I implemented the key validation! While I check for , inside the key, I do not check for = and therefore the following command fails validation.

$ fakedata --format sql --limit 1 login=email referral=domain
Unknown generator: login=email.Unknown generator: referral=domain.

See fakedata --generators for a list of available generators.

The error is also no longer multilined.

Since there's two ways of declaring the syntax I think it might be best to extract the parameter "parsing" into its own function, maybe this function could be shared with the rest of the code? This way there'd be only one spot where parameter splitting/"parsing" is defined and done.

@lucapette lucapette added the bug label May 15, 2017

@lucapette

This comment has been minimized.

Show comment
Hide comment
@lucapette

lucapette May 15, 2017

Owner

Ops we broke something :)

I remember testing the multiline error. I agree it's a good idea to extract the parsing code to a private function and share it (I believe we may need a new type but that's fine). Would you like to take care of this? Otherwise I go ahead and take care of it. No strings attached!

Owner

lucapette commented May 15, 2017

Ops we broke something :)

I remember testing the multiline error. I agree it's a good idea to extract the parsing code to a private function and share it (I believe we may need a new type but that's fine). Would you like to take care of this? Otherwise I go ahead and take care of it. No strings attached!

@kevingimbel

This comment has been minimized.

Show comment
Hide comment
@kevingimbel

kevingimbel May 15, 2017

Collaborator

I'd love to give it a try, tho I'm not sure when I get the time to do it. I'm able to do some work tomorrow on my (quite long) train ride to work - maybe I'll have a first PR ready then. Otherwise probably Wednesday or Thursday night if that's okay.

I agree it's a good idea to extract the parsing code to a private function and share it (I believe we may need a new type but that's fine)

Maybe something like a helper type? How would we share a private function? Something like the following?

type helper struct {}

func (h helper) parseInput(input str) (err error) {
 // code
}

This way we could also collect the Errors in the helper and then execute helper.GetErrors() or similar.

Collaborator

kevingimbel commented May 15, 2017

I'd love to give it a try, tho I'm not sure when I get the time to do it. I'm able to do some work tomorrow on my (quite long) train ride to work - maybe I'll have a first PR ready then. Otherwise probably Wednesday or Thursday night if that's okay.

I agree it's a good idea to extract the parsing code to a private function and share it (I believe we may need a new type but that's fine)

Maybe something like a helper type? How would we share a private function? Something like the following?

type helper struct {}

func (h helper) parseInput(input str) (err error) {
 // code
}

This way we could also collect the Errors in the helper and then execute helper.GetErrors() or similar.

@lucapette

This comment has been minimized.

Show comment
Hide comment
@lucapette

lucapette May 15, 2017

Owner

sure, I think a couple of days are totally fine (I think the userbase may still be just you and me at the moment :)). Furthermore, I'm happy this happened because it pushed me for #13

About the type, sorry I think I wasn't clear. I was thinking more about something like

type spec struct {
  name string
  key string
  constraints string // naming comes from the existing code but it's not great. I never like the names I choose though...
}

func parseSpec(input string) (s spec, err error) {
  ///
}

basically parseSpec would create an instance of the struct. Organising code this way makes usage of TableDrivenTests very cool once again. And it'd be a nice side effect.

Just an idea. I guess with the PR review cycle we'll be both happy with the final result. Thank you again for giving it a try!

Owner

lucapette commented May 15, 2017

sure, I think a couple of days are totally fine (I think the userbase may still be just you and me at the moment :)). Furthermore, I'm happy this happened because it pushed me for #13

About the type, sorry I think I wasn't clear. I was thinking more about something like

type spec struct {
  name string
  key string
  constraints string // naming comes from the existing code but it's not great. I never like the names I choose though...
}

func parseSpec(input string) (s spec, err error) {
  ///
}

basically parseSpec would create an instance of the struct. Organising code this way makes usage of TableDrivenTests very cool once again. And it'd be a nice side effect.

Just an idea. I guess with the PR review cycle we'll be both happy with the final result. Thank you again for giving it a try!

@lucapette

This comment has been minimized.

Show comment
Hide comment
@lucapette

lucapette May 15, 2017

Owner

Just a note: I fixed the error formatting here

Owner

lucapette commented May 15, 2017

Just a note: I fixed the error formatting here

@kevingimbel

This comment has been minimized.

Show comment
Hide comment
@kevingimbel

kevingimbel May 15, 2017

Collaborator

I don't get what the new spec type is supposed to do. Would it define a set of specification and separators (=, ..) that can be used for inputs? Would we parse the input into a spec to normalize it? e.g

// input: int,10..100
// spec:
func parseInput(input string) (s spec, err error) { 
  //magic
  s := spec{
    name: "int",
    key: ?
    constraint: ".."
}

return s, nil
}

Then work with the returned spec?

Can you specify a bit more what's your intention with the spec type?

Collaborator

kevingimbel commented May 15, 2017

I don't get what the new spec type is supposed to do. Would it define a set of specification and separators (=, ..) that can be used for inputs? Would we parse the input into a spec to normalize it? e.g

// input: int,10..100
// spec:
func parseInput(input string) (s spec, err error) { 
  //magic
  s := spec{
    name: "int",
    key: ?
    constraint: ".."
}

return s, nil
}

Then work with the returned spec?

Can you specify a bit more what's your intention with the spec type?

@lucapette

This comment has been minimized.

Show comment
Hide comment
@lucapette

lucapette May 15, 2017

Owner

OK, I spent some minutes on the issue and I think there's no point in introducing a new type (it's hard for me to prototype something via comments... Im finding out that today :)) as the method fakedata.NewColumns does exactly that magic for all the columns so we could just use that method and skip creating a new type. Now that I look at the attributes of the spec type I was talking about, it looks funny because it's exactly fakedata.Column.

There are a couple of options I think:

  • We change the signature of the ValidateGenerators (and the name too) to ValidateColumns(columns Columns) error
  • Or we let NewColumns handle the responsibility of validating the input. That means changing the return type to (columns Columns, err error) and get rid of the ValidateGenerators we just introduced.

I think I like both solutions almost equally, just a small preference for the second option as I can't really see why we would need Columns without knowing if we can generate things for them.

I hope it's clearer now, if not just ask more :) And sorry for all the noise about the spec type!

Owner

lucapette commented May 15, 2017

OK, I spent some minutes on the issue and I think there's no point in introducing a new type (it's hard for me to prototype something via comments... Im finding out that today :)) as the method fakedata.NewColumns does exactly that magic for all the columns so we could just use that method and skip creating a new type. Now that I look at the attributes of the spec type I was talking about, it looks funny because it's exactly fakedata.Column.

There are a couple of options I think:

  • We change the signature of the ValidateGenerators (and the name too) to ValidateColumns(columns Columns) error
  • Or we let NewColumns handle the responsibility of validating the input. That means changing the return type to (columns Columns, err error) and get rid of the ValidateGenerators we just introduced.

I think I like both solutions almost equally, just a small preference for the second option as I can't really see why we would need Columns without knowing if we can generate things for them.

I hope it's clearer now, if not just ask more :) And sorry for all the noise about the spec type!

@kevingimbel

This comment has been minimized.

Show comment
Hide comment
@kevingimbel

kevingimbel May 16, 2017

Collaborator

Prototyping pseudo code in comments is really hard. :D The second option makes more sense to me.

So, to clarify, we'd do the following:

  • Change NewColumns signature to (columns Columns, err error)
  • "add" the logic of ValdidateGenerators into NewColumns
  • Remove ValidateGenerators
  • Add the error check to main in cmd/fakedata.go (here)
  • Add / adjust the tests for NewColumns so it covers wrong generators

Does this look good to you? Did I miss anything?

Collaborator

kevingimbel commented May 16, 2017

Prototyping pseudo code in comments is really hard. :D The second option makes more sense to me.

So, to clarify, we'd do the following:

  • Change NewColumns signature to (columns Columns, err error)
  • "add" the logic of ValdidateGenerators into NewColumns
  • Remove ValidateGenerators
  • Add the error check to main in cmd/fakedata.go (here)
  • Add / adjust the tests for NewColumns so it covers wrong generators

Does this look good to you? Did I miss anything?

@lucapette

This comment has been minimized.

Show comment
Hide comment
@lucapette

lucapette May 16, 2017

Owner

It looks great! Thank you so much for taking care of it!

Owner

lucapette commented May 16, 2017

It looks great! Thank you so much for taking care of it!

@kevingimbel

This comment has been minimized.

Show comment
Hide comment
@kevingimbel

kevingimbel May 19, 2017

Collaborator

@lucapette The PR is ready for review! #15

Collaborator

kevingimbel commented May 19, 2017

@lucapette The PR is ready for review! #15

@kevingimbel

This comment has been minimized.

Show comment
Hide comment
@kevingimbel

kevingimbel May 19, 2017

Collaborator

This issue has been resolved in #15 and was fixed by merge 078593f

Collaborator

kevingimbel commented May 19, 2017

This issue has been resolved in #15 and was fixed by merge 078593f

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment