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

WIP: Generate separate data and common files. #25

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
7 participants
@dmitshur
Copy link
Member

dmitshur commented Dec 23, 2016

The goal is to resolve #23, to make it possible to use vfsgen multiple times in same package.

Previously, that would cause an error because the same types would be defined in each generated file.

The approach to solve that is to generate two files:

  • VFS data file
  • common type definitions file

This avoids the multiple declaration issues in an efficient way.

A downside is having 2 files as generated output, instead of one.

Generate separate data and common files.
The goal is to resolve #23, to make it possible to use vfsgen multiple
times in same package.

Previously, that would cause an error because the same types would be
defined in each generated file.

The approach to solve that is to generate two files:

-	VFS data file
-	common type definitions file

This avoids the multiple declaration issues in an efficient way.

A downside is having 2 files as generated output, instead of one.
@dmitshur

This comment has been minimized.

Copy link
Member Author

dmitshur commented Dec 23, 2016

Some open questions remaining:

  1. Small question. What to name the generated common file. I went with "vfsgencommon.go", is that okay? It should always be the same name so that multiple invocations of vfsgen don't result in multiple common files (the goal is to always have just one per package).

  2. Big question. It currently changes the behavior to always generate 2 files. Is that the way to go? Or is it better to make the "common" file an optional feature that user has to opt in, otherwise use previous behavior of generating one file?

    Giving user control makes for a harder to use API (they have to understand what's correctly enable the common file when needed), and complicates the implementation because there are 2 different generation paths. But if the majority of users don't need common file, it's nicer to have just 1 generated output file...

@annismckenzie

This comment has been minimized.

Copy link

annismckenzie commented Dec 23, 2016

💃 💃 💃
That'll make my build process a bit easier. I'm okay with two files.

@VojtechVitek

This comment has been minimized.

Copy link

VojtechVitek commented Feb 10, 2017

@shurcooL can we make this an option in vfsgen.Options{}, ie. CommonFilename? I'm not a big fan of two output files by default (personally, I'd prefer two different packages).

vfsgen.Generate(http.Dir("templates"), vfsgen.Options{
	PackageName:    "static",
	Filename:       "templates_static.go",
	CommonFilename: "vfsgen_common.go",
	BuildTags:      "production",
	VariableName:   "TemplatesFS",
}
vfsgen.Generate(http.Dir("css"), vfsgen.Options{
	PackageName:    "static",
	Filename:       "css_files_static.go",
	CommonFilename: "vfsgen_common.go",
	BuildTags:      "production",
	VariableName:   "CssFS",
}

and by default, the common stuff would be appended to a single generated file?

@neclepsio

This comment has been minimized.

Copy link

neclepsio commented Mar 10, 2017

I'm ok with a fixed file name for common parts and as many files as the number of vfsgen.Generate calls. I don't think it's necessary, but maybe you could make the common name configurable by package level variable.

@ncruces

This comment has been minimized.

Copy link

ncruces commented Feb 1, 2019

I'm sure you guys thought of this (so I'm probably misunderstanding the issue), but bear with me.

Assuming the common part stays the same (structure definitions and behavior) why isn't it put in a package in this repo and imported from however many generated files?

I mean any variability in the trailer seems to be removing some code happens not to be used (no directories, no uncompressed). Are the performance gains of bundling this worth the complexity?

I'm perfectly willing to do the legwork if any of this makes sense to you.

@ncruces

This comment has been minimized.

Copy link

ncruces commented Feb 1, 2019

I see in #63 you're considering this.
I'll have a go at this when I have the time.

@mklimuk

This comment has been minimized.

Copy link

mklimuk commented Mar 10, 2019

I totally agree with @ncruces. There is no point in regenerating structures that are fixed for every instance. And an import of the generating package in a generated file feels pretty natural to me. This is the approach they took for https://github.com/golang/protobuf for instance. I will try to submit a proposal in a few days.

@koliyo

This comment has been minimized.

Copy link

koliyo commented Apr 4, 2019

Please complete this MR, really cumbersome to work around this limitation. Thank you for working on this! 👍

@dmitshur

This comment has been minimized.

Copy link
Member Author

dmitshur commented Apr 11, 2019

I would like to complete the general idea started here, but it will happen no earlier than a few weeks from now, and I plan to start relying on modules to implement the idea. It may make sense to make it a feature of a v2 API, and keep v1 as is. I'll post updates in #23 when I make progress.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.