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

Extract option parsing #48

Merged
merged 14 commits into from
Jun 25, 2017
Merged

Extract option parsing #48

merged 14 commits into from
Jun 25, 2017

Conversation

lucapette
Copy link
Owner

Built on top of #43 and for that reason I'm opening the PR against that branch. So that understanding what I've done in the context of this one is easier to read.

There are a few unrelated changes that create some noise (renaming, some gardening) so let me explain what's the core of this change.

Before this change, fakedata would do the following:

  • Take all the args from the CLI
  • return an array of columns
  • For each row:
    • parse the options of each arg
    • generate a value based on the constraint

This change affects the algorithm so that we do parsing of the options only once. After the change:

  • Take all the args from the CLI
  • Parse options for each arg
  • Return a column with its own Generator embedded (that's the key change)
  • For each row:
    • generate a value using the "column generator"

It's a lot of changes but I believe it leads to better design. The two pass-algorithm removes the need for caches (like the fileCache we introduced in the file generator) and opens the door to any optimization we'd like to do in the future (in case it's needed).

I introduced a private factory too, as a consequence of this conversation which removed the init func we have.

@lucapette lucapette mentioned this pull request Jun 5, 2017
7 tasks
@@ -16,66 +14,51 @@ import (

// A Generator is a func that generates random data along with its description
type Generator struct {
Func func(Column) string
Func func() string
Copy link
Owner Author

Choose a reason for hiding this comment

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

This is the main reason why I'm happier with this design that the current master :)

return list[rand.Intn(len(list))]
gen.Func = customFn

return gen, err
}
Copy link
Owner Author

Choose a reason for hiding this comment

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

The diff doesn't help here. But getGenerator (which I'm thinking of renaming into newGenerator but always not sure about naming) is the core of the change.

We fetch the generator from the factory and the switch does all the work for the "generators with customOptions. I think over time we can make this even "more obvious" (using interfaces and structs) but it's good enough for me now that we have only 3/4 generators with "custom needs"

@lucapette lucapette added this to the v1.0.0 milestone Jun 5, 2017
lucapette referenced this pull request Jun 7, 2017
This commit adds the abillity to use cli pipes to pipe templates into
`fakedata`. It also adds all generators as named functions to the
template funcMap.
@lucapette lucapette mentioned this pull request Jun 22, 2017
@lucapette lucapette changed the base branch from new-syntax to master June 22, 2017 14:38
@lucapette lucapette mentioned this pull request Jun 23, 2017
@lucapette lucapette added the wip label Jun 23, 2017
@lucapette lucapette removed the wip label Jun 25, 2017
@lucapette lucapette merged commit 98f15f6 into master Jun 25, 2017
@lucapette lucapette mentioned this pull request Jun 25, 2017
4 tasks
@lucapette lucapette deleted the extract-option-parsing branch April 6, 2022 21:12
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.

None yet

1 participant