[WIP] Template Support #49

KevinGimbel · 2017-06-08T15:29:27Z

This pull requests adds a template functionality to fakedata. The specs and some discussion can be seen in #45.

The new template functionality supports CLI pipes (demo asciinema here) and reading in a template file (demo asciinema here).

As of now this pull request is marked as Work in Progress (WIP) because work is not yet done.

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.

KevinGimbel · 2017-06-08T16:45:42Z

pkg/fakedata/template.go

+
+// ParseTemplate takes a path to a template file as argument. It parses the template file and executes it on
+// os.Stdout.
+func ParseTemplate(path string, limit int) (err error) {


@lucapette do you have an idea how we could test this function? It needs a file on disk. Should we provide a file and if so, where would we place it? Together with the golden files for the integration tests?

KevinGimbel · 2017-06-13T06:23:53Z

@lucapette could you do a review and answer my question regarding testing with template files? Beside the test I'm happy with the current implementation. I think it works quite well and can be merged into master.

lucapette · 2017-06-14T09:08:26Z

@KevinGimbel I have very limited access to the internet for a few days more. I'm sorry this is waiting for me. I'll do my best to review it as soon as I can!

KevinGimbel · 2017-06-14T09:09:18Z

@lucapette good to know! I'll start writing the docs if I get the time to do so and maybe change a few bits in the implementation here and there.

KevinGimbel · 2017-06-14T11:42:13Z

I switched from using html/template to text/template. The html/template package has build-in escaping etc. for working with HTML files, but that is actually a disadvantage for us.

When you have < in a custom template it gets escaped and can produce issues, e.g.:

echo "#{{ Int 1 100}} {{ Name }} <{{ Email }}>" | fakedata

will produce the following output:

#45, Gaylord Phillips &lt;[email protected]>

You can see that < has turned to < because it was escaped.

As far as I can see there is not much difference between html/template and text/template beside the added security/escaping when working wit real HTML templates. In my opinion it's safer to use text/template for our use case.

lucapette · 2017-06-14T11:57:19Z

@KevinGimbel something is wrong with commenting (as I did answer the question about testing with templates).

At first sight, there's only the minor things I've commented (probably it's a good idea to merge master here as there's a conflict).

lucapette · 2017-06-14T12:00:14Z

@KevinGimbel OK I've just realised there's something else I didnt' notice. The parse methods are printing too (which makes the naming somehow a bit wrong), the problem with that is that now we have the same loop in multiple places. I don't really mind the duplication (as it's really little code) but I think the code would benefit from reorganizing things in a way that the parse methods and the GenerateRow return "something to loop *limitFlag times on and print to stdout". If that's not clear (I'm short on time/connection sadly...), we can postpone that and I can take care of it after merge. I'm almost "thinking out loud" right now :)

KevinGimbel · 2017-06-14T12:55:27Z

@lucapette I think I understand what you mean with the double loop/print and how to better handle it - I'll give this some thought.

As for the merge with master: Yes! I agree it's about time to update this branch with the latest changes from master. I'll do this as soon as I find the time.

KevinGimbel · 2017-06-18T10:33:12Z

I merged master into this feature branch and resolved the conflicts in main.go.

Next I will re-think the function naming / implementation to make it more readable since ParseTemplate also executes the template.

KevinGimbel · 2017-06-18T10:45:14Z

Of course I forgot to adjust the golden file(s) for integration tests. 🙄

Test should now succeed.

KevinGimbel · 2017-06-18T11:43:18Z

The template feature is now documented inside the README.md file. Could you proof read what I wrote? Anything missing? @lucapette @jorinvo

KevinGimbel · 2017-06-18T12:00:30Z

I re-wrote the ParseTemplate and ParseTemplateFromPipe to only parse the tempalte and return (template *template.Template, error). The previously private method executeTemplate is now exported and takes a (parsed) template as well as a limit and then loops and writes to os.Stdout. (See 1a00f08)

lucapette · 2017-06-18T12:32:37Z

@KevinGimbel the README additions look good to me at first sight! I think it's a great starting point for the rewriting of the README I plan to do as soon as all the work in progress we have right now lands into master.

The build is still red (errcheck is always checking on us! I like that :)). Apart from the red build, there are just a couple of minor things that I pointed out in comments I'd like to address before we merge (I can take care of it of course if you don't want to/don't have time).

This is going to be a great feature! 😍

KevinGimbel · 2017-06-18T13:19:59Z

Apart from the red build, there are just a couple of minor things that I pointed out in comments

In your previous comments or did you add new comments? When I look at the commits I don't see any new comments.

…n main.go

lucapette · 2017-06-18T15:46:39Z

main.go

@@ -49,13 +51,20 @@ func generatorsHelp() string {
 	return buffer.String()
 }

+func isPipe() bool {
+	stat, _ := os.Stdin.Stat()


why wouldn't we check the error here? (Honest question, I don't know the answer)

I didn't check during testing if the method works and then forgot to handle it. I'll add error checking and exit if there is an error with reading pipes (makes sense).

lucapette · 2017-06-18T15:47:07Z

main.go

 func main() {
 	var (
 		generatorsFlag = flag.BoolP("generators", "g", false, "lists available generators")
 		limitFlag      = flag.IntP("limit", "l", 10, "limits rows up to n")
 		formatFlag     = flag.StringP("format", "f", "", "generators rows in f format. Available formats: csv|tab|sql")
 		versionFlag    = flag.BoolP("version", "v", false, "shows version information")
 		tableFlag      = flag.StringP("table", "t", "TABLE", "table name of the sql format")
+		templateFlag   = flag.StringP("template", "", "", "Use template as input")


I think I'd add -T as a shortcut

Will do! I tried -t but that's the one for --table. -T should work.

lucapette · 2017-06-18T15:48:02Z

main.go

+		tmp = t
+	}
+	// Execute a template if there is one
+	if tmp != nil {


maybe let's call tmp tmpl? tmp always reminds me of temporary and I think (my guess though) it's a common association

I agree that tmp sounds more like temporary. However, tmpl throws a misspell warning with the Go tools (and with my IDE). Maybe we need to go with t? I'm currently not sure if misspells break ci or not.

lucapette · 2017-06-18T15:48:34Z

pkg/fakedata/template.go

+		if i%2 != 0 {
+			return true
+		}
+		return false


return i%2 !=0 should do it right? (I didn't check though)

Yah that should work. No idea why I went with if here. 😂

lucapette · 2017-06-18T15:52:01Z

@KevinGimbel I think I still don't really know how to use the review feature here. I have to study it a bit. So what I did is adding again all the comments I left (plus some other minor things). It's all small things but I think worth tackling.

I remember we discussed adding integration tests here (the feature calls for that kind of testing in my view) but I'm not sure what's the status of that. Do you still want to take care of that? If not we can still merge this and then I take care of it in a separate pull request (I'm fine with both options so no strings attached).

KevinGimbel · 2017-06-19T06:44:51Z

@lucapette

I think I still don't really know how to use the review feature here. I have to study it a bit. So what I did is adding again all the comments I left (plus some other minor things). It's all small things but I think worth tackling.

It's worth the time! GitHub's Review tool is pretty good in my opinion. Now I see all your comments and I've answered them.

I remember we discussed adding integration tests here (the feature calls for that kind of testing in my view) but I'm not sure what's the status of that. Do you still want to take care of that?

I agree it screams for integration tests but I have no idea how I would add them and how I would test generation with templates, etc.

If not we can still merge this and then I take care of it in a separate pull request (I'm fine with both options so no strings attached).

However you want to handle it. I need to read up on integration tests before I can implement them because I've never done it on my own - I assume I'd add tests to the cli tests and some golden files, but I have seriously no idea how I cans/should tests the templates.

KevinGimbel · 2017-06-20T11:34:27Z

@lucapette I fixed some things in 2992b97

use -T as template shortcode
renamed tmp variable to tmpl
remove if/else from Odd and Even funcs
handle error in isPipe

Additionally:

Handle file read error in File template func
Add test for non-existing file

From my point of view only the Integration tests needs to be done, otherwise the template feature is ready. 🎉

lucapette · 2017-06-20T14:40:11Z

@KevinGimbel thank you for all the changes! It looks great now!

I suggest we don't merge it before we're able to add integration tests. I already started working on it so I will push them directly to this branch and ping you when I'm done.

Can't wait to land this feature!

KevinGimbel · 2017-06-20T15:46:42Z

@lucapette Exciting! I can't wait either, it's been a lot of fun working on this. :)

Expose only a method that takes a template

lucapette · 2017-06-22T14:35:20Z

@KevinGimbel I finally found some time to work on the feature. I tried to simplify the API as much as I could (therefore did some code removing) and introduced the integration tests we talked about. I'm satisfied with the coverage and the current status of the feature so I'm going to merge it as it is now 🎉

It's going to be hard to make this work with #48 but I can't wait to give that a try!

KevinGimbel added 3 commits June 1, 2017 13:22

(template): WIP: Inital version. Updates #45

4f9d978

(template): Add & Update Tests; Rewrite Helper funcs; Updates #45

89cab85

UPDATES #45; Implement pipe & template

5508aa1

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.

KevinGimbel mentioned this pull request Jun 8, 2017

Proposal: Template functionality #45

Closed

lucapette added the wip label Jun 8, 2017

lucapette self-requested a review June 8, 2017 15:56

lucapette assigned KevinGimbel Jun 8, 2017

(test): Add tests; Fix formatting of log messages

8f2a6d0

KevinGimbel commented Jun 8, 2017

View reviewed changes

(template): Ass isPipe func for clarity

21388c5

(tempalte): switch from html/template to text/template

04838d6

(template): Update branch with changed from master

8202080

(ci): Fix integration test file unknown-format.golden

1464a8e

(docs): Add template documentation to README

bcca53e

(template): Expose ExecuteTemplate; Seperate parse and execute

1a00f08

(template): Add test for ExecuteTemplate; use variable for template i…

c105af8

…n main.go

lucapette reviewed Jun 18, 2017

View reviewed changes

(tempalte): Handle error in File func; Add Tests; Handle Error in isPipe

2992b97

(housekeeping): Minor text correction in README

5cb8cb7

lucapette removed the wip label Jun 22, 2017

lucapette added this to the v1.0.0 milestone Jun 22, 2017

lucapette added 6 commits June 22, 2017 16:32

Simplify little things

ac00315

First working integration test for templates

bb2754e

More coverage

37b4202

Cover pipe templates too

1e05268

Simplifiy code

1d8890a

Simpler API for fakedata

300eedf

Expose only a method that takes a template

lucapette merged commit 8c4a8f3 into master Jun 22, 2017

lucapette deleted the feat/45/templateGenerator branch April 6, 2022 21:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Template Support #49

[WIP] Template Support #49

KevinGimbel commented Jun 8, 2017

KevinGimbel Jun 8, 2017

KevinGimbel commented Jun 13, 2017

lucapette commented Jun 14, 2017

KevinGimbel commented Jun 14, 2017

KevinGimbel commented Jun 14, 2017

lucapette commented Jun 14, 2017

lucapette commented Jun 14, 2017

KevinGimbel commented Jun 14, 2017

KevinGimbel commented Jun 18, 2017

KevinGimbel commented Jun 18, 2017

KevinGimbel commented Jun 18, 2017

KevinGimbel commented Jun 18, 2017

lucapette commented Jun 18, 2017

KevinGimbel commented Jun 18, 2017

lucapette Jun 18, 2017

KevinGimbel Jun 19, 2017

lucapette Jun 18, 2017

KevinGimbel Jun 19, 2017

lucapette Jun 18, 2017

KevinGimbel Jun 19, 2017

lucapette Jun 18, 2017

KevinGimbel Jun 19, 2017

lucapette commented Jun 18, 2017

KevinGimbel commented Jun 19, 2017

KevinGimbel commented Jun 20, 2017

lucapette commented Jun 20, 2017

KevinGimbel commented Jun 20, 2017

lucapette commented Jun 22, 2017

[WIP] Template Support #49

[WIP] Template Support #49

Conversation

KevinGimbel commented Jun 8, 2017

Choose a reason for hiding this comment

KevinGimbel commented Jun 13, 2017

lucapette commented Jun 14, 2017

KevinGimbel commented Jun 14, 2017

KevinGimbel commented Jun 14, 2017

lucapette commented Jun 14, 2017

lucapette commented Jun 14, 2017

KevinGimbel commented Jun 14, 2017

KevinGimbel commented Jun 18, 2017

KevinGimbel commented Jun 18, 2017

KevinGimbel commented Jun 18, 2017

KevinGimbel commented Jun 18, 2017

lucapette commented Jun 18, 2017

KevinGimbel commented Jun 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucapette commented Jun 18, 2017

KevinGimbel commented Jun 19, 2017

KevinGimbel commented Jun 20, 2017

lucapette commented Jun 20, 2017

KevinGimbel commented Jun 20, 2017

lucapette commented Jun 22, 2017