Use vfsgen to statically embed template data. #84
Conversation
|
I've tried to keep the filenames unchanged to make it easier to review.
|
|
Travis CI is failing, but not sure if that's related to my changes or external. The last |
|
It turns out Travis is passing on |
|
@shurcooL I suspect perhaps the filepaths have changed somehow, but can't say for certain |
|
On second thought, this appears unrelated to your change. Running |
|
The Makefile that was in the root folder previous to being removed in this PR was causing Travis not to run tests at all, that's why it succeeded. See the bottom of https://travis-ci.org/sourcegraph/appdash/jobs/79379987 output:
|
These tests were failing (but not reported by Travis due to the Makefile, soon to be removed by #84) due to the fact that the keys changed previously and are now prefixed with Server/Client for easy differentiation in the web UI.
Remove deprecated Makefile build process. Replace use of go-bindata with vfsgen for embedding template data. As a side effect, this removes the need to gofmt the generated output. Move go generate directives into ./traceapp/tmpl package, since that's the only package they affect.
|
Done, and it's green now. |
| return fmt.Errorf("template %v: %s", set, err) | ||
| } | ||
| tmplBytes, err := ioutil.ReadAll(tmplFile) | ||
| tmplFile.Close() |
There was a problem hiding this comment.
As an observation, if this were my codebase, I would use a helper similar to ioutil.ReadFile but for virtual filesystems:
tmplBytes, err := vfsutil.ReadFile(tmpl.Assets, "/" + tmp)
...Then the number of lines would not increase here. But I figured it's not worth pulling in an extra package just to replace a standard library package and 4 extra lines of standard code.
There was a problem hiding this comment.
Yeah, I think that would make sense if we did it in more than a few places -- but IMO it's a bit easier to read these standard lines rather than not strictly knowing what vfsutil.ReadFile (despite obvious name) does.
|
LGTM |
Use vfsgen to statically embed template data.
go generatedirectives into./traceapp/tmplpackage, since that's the only package they affect.Rationale
Reasons for switching to
vfsgeninclude:go fmt ./...after regeneration.http.FileSystemdirectly matches the needs of this project (bonus of being done in one package instead of across two separately maintained packages).