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

go gen improvements #524

Merged
merged 1 commit into from
Feb 14, 2017
Merged

go gen improvements #524

merged 1 commit into from
Feb 14, 2017

Conversation

miekg
Copy link
Member

@miekg miekg commented Feb 14, 2017

Remove the "gen" directory and move directives_generate.go out of it.
Add a build ignore tag so it isn't build by default. Cleanup the go ge
invocations so there are not seen as package docs.

@miekg
Copy link
Member Author

miekg commented Feb 14, 2017

@bwaz I'm still fuzzy on why this 'go gen' needs to be called twice. Why is that?

@codecov-io
Copy link

codecov-io commented Feb 14, 2017

Codecov Report

Merging #524 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #524   +/-   ##
=======================================
  Coverage   38.88%   38.88%           
=======================================
  Files         120      120           
  Lines        5995     5995           
=======================================
  Hits         2331     2331           
  Misses       3479     3479           
  Partials      185      185
Impacted Files Coverage Δ
core/dnsserver/directives.go 0% <ø> (ø)

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 a5f3cb5...512109c. Read the comment docs.

@bwaz
Copy link
Contributor

bwaz commented Feb 14, 2017

go generate will provide the path and package name that where it was called from. So if the package layout changes the generated files will change package name and location path as well. The imports and directives need to parse the same config file to provide output for each use.

@miekg
Copy link
Member Author

miekg commented Feb 14, 2017

So the changes I've just pushed, will make directives_generate.go break somehow? I'm sorry, but I think I still don't see it. If you can show me some change/code that will break this PR I'm happy to rollback.

Makefile Outdated
@@ -50,7 +50,7 @@ clean:

.PHONY: gen
gen:
go generate ./core/...
go generate
Copy link
Contributor

Choose a reason for hiding this comment

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

make sure this actually will generate the z files; delete them first and retry.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've tried go generate ./... however this caused issues with vendor code - you could for now just explicitly generate the coredns.go

Copy link
Member Author

Choose a reason for hiding this comment

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

done. Listed coredns.go explicitly.

Copy link
Member Author

Choose a reason for hiding this comment

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

also removed the z* files and go genned - that works.

}
}

var orders []int
Copy link
Contributor

Choose a reason for hiding this comment

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

This is unused and redundant what is in the func.... I missed this earlier.

Copy link
Member Author

Choose a reason for hiding this comment

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

removed.

Remove the "gen" directory and move directives_generate.go out of it.
Add a build ignore tag so it isn't build by default. Cleanup the go gen
invocations so there are not seen as package docs.

Simplify the code a bit and don't run go gen twice.
@miekg miekg merged commit 98c86f3 into master Feb 14, 2017
@miekg miekg deleted the gen-dir branch February 14, 2017 19:27
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.

3 participants