Replace data_generate.go with a proposed use of vfsgen binary. #91

dmitshur · 2015-10-18T21:42:12Z

This change makes use of a new vfsgendev command-line tool in order to embed the static assets instead of the old data_generate.go file.

The tool is opinionated and designed specifically for use cases like ours here.
New behavior is identical to the old behavior.

For more information, see shurcooL/vfsgen#16

dmitshur · 2015-12-08T22:02:36Z

Now that I've created binstale, I feel more comfortable requiring the use of a vfsgen binary, because I can now ensure it's the latest non-stale version before running go generate.

Previously, I could only do that for Go package source (via gostatus command).

Can you review the proposed API?

After taking enough time to reflect, I think I like this now and would like to proceed with finishing the vfsgen binary implementation.

dmitshur · 2015-12-25T08:21:16Z

Can you review the proposed API?

Friendly ping, @slimsag.

slimsag · 2015-12-25T18:24:56Z

traceapp/tmpl/data.go

 	}
 	return p.Dir
 }

 // Data is a virtual filesystem that contains template data used by Appdash.
-var Data = http.Dir(filepath.Join(defaultBase("sourcegraph.com/sourcegraph/appdash/traceapp/tmpl"), "data"))
+var Data = http.Dir(importPathToDir("sourcegraph.com/sourcegraph/appdash/traceapp/tmpl/data"))


This file change == very good.

Yep. Although it's not related to the vfsgen binary change, this is just updating this code to the latest version of this pattern. I should've made it a separate PR, that way it could've been merged much sooner (though you're welcome to cherry pick it yourself).

slimsag · 2015-12-25T18:32:00Z

A few questions, @shurcooL, but overall this looks like a good simplification. I really like the direction this is going in!

slimsag · 2015-12-30T00:13:03Z

@shurcooL everything you said made sense, I agree that my proposal for automatic detection of the var would be a bit magical. I have another proposal:

Make the default for -output-tags be -output-tags=!dev.
Make the default for -tags be -tags=dev.

I wanted to hold off on proposing this to you until I had a chance to confirm that this would be a good idea, but gopherjs/gopherjs#369 has reconfirmed my thoughts that it might be good.

This would make the vfsgen binary a developer-first oriented tool, i.e. assume that you'll always want a dev and non-dev VFS. In my experience, I always want this -- and in the cases that I do not use this feature it is because it is hard to set up and I don't need it that badly.

I can now point to one of my personal projects, appdash, and gopherjs all as potential users of the vfsgen binary and wanting to use the above defaults. What do you think on this?

slimsag · 2015-12-30T00:14:03Z

Also, note that I do think this is a good change. We can move forward with this current approach now if you'd like and continue discussing the default parameters elsewhere.

dmitshur · 2015-12-30T01:12:52Z

This would make the vfsgen binary a developer-first oriented tool, i.e. assume that you'll always want a dev and non-dev VFS. In my experience, I always want this -- and in the cases that I do not use this feature it is because it is hard to set up and I don't need it that badly.

I can now point to one of my personal projects, appdash, and gopherjs all as potential users of the vfsgen binary and wanting to use the above defaults. What do you think on this?

I like the idea and agree, it would work for all my current needs/uses of vfsgen too.

However, I don't like the idea of mixing up general purpose flags with opinionated and convention-driven ones.

I discussed it with @slimsag on Slack and we converged taking the idea of making vfsgen binary even more opinionated, simple to use, and developer-oriented. We'll call it vfsgendev to indicate that it's not a general purpose and convention-agnostic tool. Instead, it will focus on solving the most common use case well and being as friendly/easy to use as possible.

For general purpose/custom needs, vfsgen the Go library can still be used. Let's see how vfsgendev works out and go from there.

I'll update the PR to have this API then:

-//go:generate vfsgen -tags=dev -source="sourcegraph.com/sourcegraph/appdash/traceapp/tmpl".Data -output-tags=!dev
+//go:generate vfsgendev -source="sourcegraph.com/sourcegraph/appdash/traceapp/tmpl".Data

vfsgendev will not have any other flags. I think -source is all it'll need.

Add an opinionated binary for specialized use. It uses the convention of using "dev" build tag for development mode, and assumes that the source VFS is defined in a "dev" only environment; it uses "!dev" build tag in output .go file. Usage is simple: vfsgendev -source="import/path".VariableName See sourcegraph/appdash#91 for additional background, motivation, and discussion. Resolves #2. Closes #14.

dmitshur · 2015-12-31T02:49:16Z

PTAL, this is the final API, and it's fully functional.

You will need the vfsgendev binary in your GOPATH to run go generate. It'll be generally available after shurcooL/vfsgen#16 is merged.

dmitshur · 2015-12-31T02:53:11Z

CI is failing on Go 1.3, 1.4, succeeding on tip. Looks completely unrelated to this change (this change affects generate time, not build or run time).

slimsag · 2015-12-31T04:15:49Z

re: CI failing -- yeah, I don't know what that was about, but it's passing now: https://travis-ci.org/sourcegraph/appdash/builds/99542211

If I see it happen again I'll look into it further.

slimsag · 2015-12-31T04:18:32Z

Some TODOs before merging:

Merge Add ./cmd/vfsgendev binary to address common use case. shurcooL/vfsgen#16
Update description of PR (I'm doing this now).
Squash into just two commits, as history is a bit polluted. Can you do this @shurcooL ?

LVGTM. Thank you for this, @shurcooL.

Get rid of filepath.Join since it's avoidable. Rename defaultBase to a more descriptive name.

Advantage is that code is more succinct (less boilerplate). Disadvantage is that you now need to install vfsgendev binary to be able to run go generate. Previously, only the vfsgen library was required. Behavior is unchanged.

dmitshur · 2015-12-31T05:01:54Z

Squash into just two commits, as history is a bit polluted. Can you do this @shurcooL ?

Sure. Done.

dmitshur · 2016-01-01T03:30:42Z

Merge shurcooL/vfsgen#16

Also done!

slimsag · 2016-01-11T01:35:44Z

Sorry for the delay! I thought I had merged this when you wrote your comment above.

LGTM

Replace data_generate.go with a proposed use of vfsgen binary.

dmitshur mentioned this pull request Oct 18, 2015

Add a command-line version of vfsgen shurcooL/vfsgen#14

Closed

slimsag reviewed Dec 25, 2015
View reviewed changes

dmitshur mentioned this pull request Dec 30, 2015

Create a VFS variable for native packages. gopherjs/gopherjs#369

Closed

dmitshur mentioned this pull request Dec 31, 2015

Add ./cmd/vfsgendev binary to address common use case. shurcooL/vfsgen#16

Merged

dmitshur added 2 commits December 30, 2015 20:58

traceapp/tmpl: Simplify definition of Data.

ccdf38d

Get rid of filepath.Join since it's avoidable. Rename defaultBase to a more descriptive name.

slimsag added a commit that referenced this pull request Jan 11, 2016

Merge pull request #91 from shurcooL/try-using-vfsgen-binary

84f0569

Replace data_generate.go with a proposed use of vfsgen binary.

slimsag merged commit 84f0569 into sourcegraph:master Jan 11, 2016

dmitshur deleted the try-using-vfsgen-binary branch January 11, 2016 04:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace data_generate.go with a proposed use of vfsgen binary. #91

Replace data_generate.go with a proposed use of vfsgen binary. #91

dmitshur commented Oct 18, 2015

dmitshur commented Dec 8, 2015

dmitshur commented Dec 25, 2015

slimsag Dec 25, 2015

dmitshur Dec 25, 2015

slimsag commented Dec 25, 2015

slimsag commented Dec 30, 2015

slimsag commented Dec 30, 2015

dmitshur commented Dec 30, 2015

dmitshur commented Dec 31, 2015

dmitshur commented Dec 31, 2015

slimsag commented Dec 31, 2015

slimsag commented Dec 31, 2015

dmitshur commented Dec 31, 2015

dmitshur commented Jan 1, 2016

slimsag commented Jan 11, 2016

Replace data_generate.go with a proposed use of vfsgen binary. #91

Replace data_generate.go with a proposed use of vfsgen binary. #91

Conversation

dmitshur commented Oct 18, 2015

dmitshur commented Dec 8, 2015

dmitshur commented Dec 25, 2015

slimsag Dec 25, 2015

Choose a reason for hiding this comment

dmitshur Dec 25, 2015

Choose a reason for hiding this comment

slimsag commented Dec 25, 2015

slimsag commented Dec 30, 2015

slimsag commented Dec 30, 2015

dmitshur commented Dec 30, 2015

dmitshur commented Dec 31, 2015

dmitshur commented Dec 31, 2015

slimsag commented Dec 31, 2015

slimsag commented Dec 31, 2015

dmitshur commented Dec 31, 2015

dmitshur commented Jan 1, 2016

slimsag commented Jan 11, 2016