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

Add RenderWithFile support #56

Merged
merged 1 commit into from
Jan 29, 2019
Merged

Add RenderWithFile support #56

merged 1 commit into from
Jan 29, 2019

Conversation

dsxack
Copy link
Contributor

@dsxack dsxack commented Jan 29, 2019

Fix for #55

@codecov-io
Copy link

codecov-io commented Jan 29, 2019

Codecov Report

Merging #56 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #56   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          14     14           
  Lines        1706   1708    +2     
=====================================
+ Hits         1706   1708    +2
Impacted Files Coverage Δ
jen/statement.go 100% <100%> (ø) ⬆️
jen/group.go 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0c10b93...b817c4e. Read the comment docs.

@dave
Copy link
Owner

dave commented Jan 29, 2019

OK this looks good, and it's a valid addition. A few things:

  1. Don't need GoStringWithFile... GoString is called with the fmt package formats with the #v verb, so adding a variation doesn't make sense.

  2. We should tweak the internals of the Render function to use RenderWithFile(writer, f) instead of duplicating all the logic.

  3. The Group type has a Render method too... it should probably have RenderWithFile also.

Thanks for the PR!

@dsxack
Copy link
Contributor Author

dsxack commented Jan 29, 2019

Thank you for review! I'm going to fix remarks now

@dsxack
Copy link
Contributor Author

dsxack commented Jan 29, 2019

I've done your PR comments

jen/group.go Outdated Show resolved Hide resolved
jen/statement.go Outdated Show resolved Hide resolved
jen/group_test.go Outdated Show resolved Hide resolved
jen/statement.go Outdated Show resolved Hide resolved
jen/group.go Outdated Show resolved Hide resolved
jen/statement.go Outdated Show resolved Hide resolved
jen/group.go Outdated Show resolved Hide resolved
jen/statement.go Outdated Show resolved Hide resolved
@dave dave merged commit 14e399b into dave:master Jan 29, 2019
@dave
Copy link
Owner

dave commented Jan 29, 2019

Merged! Thanks!

@dsxack
Copy link
Contributor Author

dsxack commented Jan 29, 2019

Thank you!

@dave
Copy link
Owner

dave commented Jan 29, 2019

I've tagged this release as v1.3.0.

Thanks very much!

@dsxack dsxack deleted the render_statement_with_passed_file branch January 29, 2019 20:32
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

3 participants