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

feat: add -o flag to override the output file for a single input file #735

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jespert
Copy link

@jespert jespert commented May 13, 2024

While this could be seen as redundant given the existing flag -stdout, redirecting stdout is difficult in some environments such as Bazel (bazelbuild/bazel#5511).

Expected behaviour

Starting point:

$ ls
foo.templ  go.mod  go.sum out/
$ cat foo.templ 
$ cat go.mod 
module foo

go 1.22.2

require github.com/a-h/templ v0.2.680 // indirect

New behaviour:

  • templ generate -f foo.templ -o bar_templ.go generates bar_templ.go.
  • More interestingly for Bazel, templ generate -f foo.templ -o out/bar_templ.go generates out/foo_templ.go.
  • Setting -o without -f returns an error stating the missing requirement.
  • Simultaneously setting -o and -stdout returns an error citing the incompatibility.
  • Setting an output filename that doesn't end in _templ.go returns an error.

@jespert
Copy link
Author

jespert commented May 13, 2024

Hi, folks!

I didn't start a discussion because it seemed a small enough feature not to warrant the "major" piece of work described in CONTRIBUTING.md, but please do let me know if you prefer it that way.

Thanks in advance for your time.

PS: I have only been using Templ for a few days, but I'm very impressed so far. Nice job!

@jespert
Copy link
Author

jespert commented May 15, 2024

I had somehow overlooked #198. Sorry. It's discussion issue seems closed, so I'll carry on here.

While I totally understand the reticence to allow generating files in a separate package, let me expand a bit on the Bazel situation.

Due to how Bazel works, generated files need to be written to a directory different to that of the source, even if they conceptually belong there. And in fact when you try to use the generated source files as targets for other rules (e.g., to build a Go library), it will be as if they are there. Just not during generation. Having -stdout offers some respite here, but then we get to the second problem: the support for redirection in Bazel is very limited, and forces you to go with shell scripts that you might have to customise per platform (e.g., can't assume that Bash is everywhere). I believe that the fix really belongs in Bazel, but it's an unrealistic target for me. Hence why I implemented a rather minimal support in templ that I hoped wouldn't be too offensive, even if I'm not the biggest fan of -o flags myself.

Ultimately, I'd like to create reusable Bazel rules for templ to remove as much of the hackery as possible at the point of usage. Possibly even integrate them with Gazelle (which autogenerates Bazel targets), and ensure that the templ binary tracks the version in go.mod. But I'm happy to scale down my ambitions here and go with a much more limited solution here if you do believe that -o isn't really worth it. I only need Linux, and writing the Bazel rules was just a side-quest on some other project anyway. 😁

Thanks for your time.

@a-h
Copy link
Owner

a-h commented May 26, 2024

Thanks for the PR, sorry it's got conflicts now - but to be fair, that's because of the stdout -> stderr issue. 😁

I'm up for adding the feature, but I think we need a suite of tests to cover all of the scenarios, or it will introduce flakiness. The existing unit test suite covers the standard use cases, so up until now there hasn't been a specific suite of tests for generatecmd.

So to get us started, I added one:

package generatecmd

import (
	"context"
	"io"
	"log/slog"
	"os"
	"path"
	"testing"

	"github.com/a-h/templ/cmd/templ/testproject"
)

func TestGenerate(t *testing.T) {
	log := slog.New(slog.NewJSONHandler(io.Discard, nil))
	t.Run("can generate a file in place", func(t *testing.T) {
		// templ generate -f templates.templ
		dir, err := testproject.Create("github.com/a-h/templ/cmd/templ/testproject")
		if err != nil {
			t.Fatalf("failed to create test project: %v", err)
		}
		defer os.RemoveAll(dir)

		// Delete the templates_templ.go file to ensure it is generated.
		err = os.Remove(path.Join(dir, "templates_templ.go"))
		if err != nil {
			t.Fatalf("failed to remove templates_templ.go: %v", err)
		}

		// Run the generate command.
		err = Run(context.Background(), log, Arguments{
			FileName: path.Join(dir, "templates.templ"),
		})
		if err != nil {
			t.Fatalf("failed to run generate command: %v", err)
		}

		// Check the templates_templ.go file was created.
		_, err = os.Stat(path.Join(dir, "templates_templ.go"))
		if err != nil {
			t.Fatalf("templates_templ.go was not created: %v", err)
		}
	})
}

I also added another example test in the format command as a reference.

We need to support:

  • Input file, output file (that's what the test above demonstrates)
  • Input stdin, output file (not currently supported)
  • Input file, output stdout (not currently supported)
  • Input dir, output dir (defaults to same directory) - currently hard coded to same as input dir, without a specific unit test

The CLI design you did is good, adding the additional -o and -stdout flags to support output file/dir, and whether to use stdout is good.

This change makes another issue though. The templ lsp command would also need a output dir param, and the filenames remapped within the LSP in the same way as the templ generate command or the LSP wouldn't work for people that use the output flag to set an output directory.

I was reluctant to do something with this because it adds a number of combinations of behaviour which need to be tested, and may cause negative interactions with each other. Basically, worried that it would produce a lot of support work for what I think is potentially a very niche use case. I suspect it's <1% of Go users?

But... if you and @rowan-gud (author of #198) are up for working together on getting the support together for this, complete with tests, I'm willing to integrate the changes.

@jespert
Copy link
Author

jespert commented May 28, 2024

Challenge accepted. 😁 (just let me know if you want in, @rowan-gud!)

Agree on the testing concerns. Thanks for highlighting the LSP angle, which I was unaware of, and for the test. I also totally get you on complexity. I'm hoping that we can isolate the generation mechanism from the input/output file selection, maybe not just on the code but also on the tests. I recall seeing some potential opportunities for refactoring, but I won't know for sure until I try. To be honest with you, I had opened GitHub this morning to close the MR since I have my end sorted out in other ways, but this is a great development and I'm happy to delve deeper into the problem if you're willing to review the proposals. And on that front, I'm very much a CI/CD kind of guy and would prefer doing these changes incrementally rather than try to deliver a huge feature at once. Would you be happy with that?

It's true that we don't know the adoption rate of Bazel, Make or other build tools in Go because they aren't part of the Go Developer Survey, but most commercial projects that I have worked on use one of the first two in some capacity. After a while, you often need to generate code, data and/or derived artefacts (container images, gRPC or OpenAPI specs/clients/servers, etc.), and that's where only using the Go toolchain ends. My hunch (and experience) is that they are very rarely used in libraries, infrequently used in opensource applications, and frequently used on startups and enterprises (e.g., Google, Uber, etc.). Features such as logging to stderr, or supporting flexible input/output redirection will improve the experience for all build tools, not just Bazel, which I believe will be good for Templ.

Thanks again for your time!

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

2 participants