Conversation
Owner
Author
|
When I get to your suggestions @philip-peterson, I'll be sure to give you credit! |
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the generator command to improve testability and maintainability by:
- Extracting concurrent maps/sets into their own generic types
- Replacing manual
sync.WaitGroupand error slices witherrgroupfor clearer error handling - Centralizing CLI argument parsing, error handling, and version bump
Reviewed Changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| generator/test-css-middleware/render_test.go | Switched from sync.WaitGroup and error slice to errgroup |
| generator/htmldiff/diff.go | Replaced manual goroutines and sync.WaitGroup with errgroup |
| cmd/templ/sloghandler/handler.go | Added NewLogger factory to configure slog.Logger |
| cmd/templ/main_test.go | Updated tests to match new logger and CLI behavior |
| cmd/templ/main.go | Wired in sloghandler.NewLogger and removed inline logger setup |
| cmd/templ/generatecmd/testwatch/generate_test.go | Updated watch-mode tests to use new Run signature and errgroup |
| cmd/templ/generatecmd/syncset/set_test.go | Added unit tests for new generic syncset |
| cmd/templ/generatecmd/syncset/set.go | Introduced Set[T] with mutex and basic methods |
| cmd/templ/generatecmd/syncmap/map_test.go | Added unit tests for new generic syncmap |
| cmd/templ/generatecmd/syncmap/map.go | Introduced Map[K,V] with mutex and CAS |
| cmd/templ/generatecmd/symlink/symlink_test.go | Updated symlink tests to new Run signature |
| cmd/templ/generatecmd/main_test.go | Adapted generate CLI tests to new argument parsing |
| cmd/templ/generatecmd/main.go | Centralized NewArguments, Run signature, and usage handling |
| cmd/templ/generatecmd/eventhandler.go | Replaced manual maps/mutexes with syncmap/syncset and errgroup |
| cmd/templ/generatecmd/cmd.go | Simplified NewGenerate; removed inline defaults and regex logic |
| .version | Bumped version from 0.3.906 to 0.3.908 |
Comments suppressed due to low confidence (2)
cmd/templ/generatecmd/eventhandler.go:258
- This
fmt.Printlnappears to be a stray debug statement; consider removing it or replacing with structured logging (h.Log.Debugorh.Log.Info) to keep output consistent.
// Takes an error from the formatter and attempts to convert the positions reported in the target file to their positions
cmd/templ/sloghandler/handler.go:13
- This function signature references io.Writer and slog.Logger but neither are imported; add
import "io"andimport "log/slog"(or alias) to fix the compile error.
func NewLogger(logLevel string, verbose bool, stderr io.Writer) *slog.Logger {
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Contributor
|
@a-h Thanks for doing this! If merged, looks like this will be way better than my refactors anyway. |
joerdav
reviewed
Jul 3, 2025
joerdav
reviewed
Jul 3, 2025
Collaborator
|
Nice, some real improvements here! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When reviewing @philip-peterson's PR, I was struck by the general messiness of the generator code, caused by adding on extra features over time.
My plan is to get the generator into a better testing position. At the moment, the generator is tested well by generating templ files, and comparing outputs, and the watch mode has a basic integration test to validate that it works, but it didn't catch a regression in watch mode behaviour in the current release, so I want to restructure the code to support adding some behavioural unit tests to cover more of the feature set.
So, this PR is a bit of a grab bag of general improvements: moving the concurrent maps into their own types, simplifying waitgroups with
errgroup, consolidating the argument processing for the generate command into a single area, and improving test coverage.