search

lucapette / fakedata

CLI utility for fake data generation
MIT License
200 stars 7 forks source link

[WIP] Template Support #49

Closed KevinGimbel closed 7 years ago

KevinGimbel commented 7 years ago

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.

KevinGimbel commented 7 years ago

@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 commented 7 years ago

@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 commented 7 years ago

@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 commented 7 years ago

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;giancarlon@example.rehab>

You can see that < has turned to &lt; 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 commented 7 years ago

@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 commented 7 years ago

@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 commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

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

Test should now succeed.

KevinGimbel commented 7 years ago

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

KevinGimbel commented 7 years ago

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 https://github.com/lucapette/fakedata/pull/49/commits/1a00f08bcb26927bdffb46583e9eea460d1d5403)

lucapette commented 7 years ago

@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 commented 7 years ago

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.

lucapette commented 7 years ago

@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 commented 7 years ago

@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 commented 7 years ago

@lucapette I fixed some things in https://github.com/lucapette/fakedata/pull/49/commits/2992b976b6c3d3ad63be7ecb959444c18d2509f9

Additionally:

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

lucapette commented 7 years ago

@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 commented 7 years ago

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

lucapette commented 7 years ago

@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!