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 gen #29

Merged
merged 1 commit into from Aug 7, 2022
Merged

feat: add gen #29

merged 1 commit into from Aug 7, 2022

Conversation

sashamelentyev
Copy link
Owner

No description provided.

@sashamelentyev
Copy link
Owner Author

@ldez hello!
I added generation, please review this.
Thank you!

@sashamelentyev sashamelentyev force-pushed the feat/gen branch 3 times, most recently from 18bd065 to 0dbf852 Compare August 5, 2022 12:28
@sashamelentyev
Copy link
Owner Author

Changed map to array in gen data, because map have random iteration order

@sashamelentyev sashamelentyev force-pushed the feat/gen branch 3 times, most recently from 1e5a19e to 4fcf75e Compare August 5, 2022 13:13
@ldez
Copy link
Contributor

ldez commented Aug 7, 2022

The implementation seems not right: using a hard-coded mapping to create a hard-coded mapping is not really useful.

I think instead of trying to generate the mapping and the tests inside the same pass, you have to create 2 passes/"main"/packages: one for tests (that use the real mapping) and one for mapping (this one is optional if you are not able to not use mapping to generate mapping)

Tips: to be able to use the mapping inside the generator and the analyzer without publically exporting the global vars you can just move the mapping file to the internal package and export the global vars.

@sashamelentyev sashamelentyev force-pushed the feat/gen branch 2 times, most recently from 709520b to 385f61f Compare August 7, 2022 04:23
@sashamelentyev
Copy link
Owner Author

@ldez thank you for review!
I hard-code mapping, and move file to internal/mapping. And move internal to pkg/analyzer.
Right now generate tests by real mapping only

@sashamelentyev sashamelentyev force-pushed the feat/gen branch 2 times, most recently from 1d2bd68 to a1ce817 Compare August 7, 2022 10:44
@ldez
Copy link
Contributor

ldez commented Aug 7, 2022

another tip:

// gofmt
source, err := format.Source(buffer.Bytes())
if err != nil {
	return fmt.Errorf("source: %w", err)
}

@sashamelentyev
Copy link
Owner Author

sashamelentyev commented Aug 7, 2022

@ldez , wow, thank you!
Add this.
Can I merge this?

pkg/analyzer/internal/gen.go Outdated Show resolved Hide resolved
@ldez
Copy link
Contributor

ldez commented Aug 7, 2022

Ldez not idez 😉

@sashamelentyev
Copy link
Owner Author

sashamelentyev commented Aug 7, 2022

My bad, forgive me

@ldez
Copy link
Contributor

ldez commented Aug 7, 2022

I have several suggestions.

I think you can rename template files following this pattern test-httpmethod.go.tmpl.

The code generator can be written differently:

func main() {
	t := template.Must(
		template.New("template").
			Funcs(map[string]any{"quoteMeta": regexp.QuoteMeta}).
			ParseFS(templateDir, "template/*.tmpl"),
	)

	operations := []struct {
		templateName string
		packageName  string
		mapping      map[string]string
		fileName     string
	}{
		{
			templateName: "template.tmpl",
			packageName:  "crypto_test",
			mapping:      mapping.CryptoHash,
			fileName:     "pkg/analyzer/testdata/src/a/crypto/crypto.go",
		},
		{
			templateName: "httpmethod.tmpl",
			packageName:  "http_test",
			mapping:      mapping.HTTPMethod,
			fileName:     "pkg/analyzer/testdata/src/a/http/method.go",
		},
		{
			templateName: "httpstatuscode.tmpl",
			packageName:  "http_test",
			mapping:      mapping.HTTPStatusCode,
			fileName:     "pkg/analyzer/testdata/src/a/http/statuscode.go",
		},
		{
			templateName: "template.tmpl",
			packageName:  "rpc_test",
			mapping:      mapping.DefaultRPCPath,
			fileName:     "pkg/analyzer/testdata/src/a/rpc/rpc.go",
		},
		{
			templateName: "template.tmpl",
			packageName:  "time_test",
			mapping:      mapping.TimeWeekday,
			fileName:     "pkg/analyzer/testdata/src/a/time/weekday.go",
		},
		{
			templateName: "template.tmpl",
			packageName:  "time_test",
			mapping:      mapping.TimeMonth,
			fileName:     "pkg/analyzer/testdata/src/a/time/month.go",
		},
		{
			templateName: "template.tmpl",
			packageName:  "time_test",
			mapping:      mapping.TimeLayout,
			fileName:     "pkg/analyzer/testdata/src/a/time/layout.go",
		},
	}

	for _, operation := range operations {
		data := map[string]any{
			"PackageName": operation.packageName,
			"Mapping":     operation.mapping,
		}

		err := execute(t, operation.templateName, data, operation.fileName)
		if err != nil {
			log.Fatal(err)
		}
	}
}

func execute(t *template.Template, templateName string, data any, fileName string) error {
	var builder bytes.Buffer
	if err := t.ExecuteTemplate(&builder, templateName, data); err != nil {
		return err
	}

	sourceData, err := format.Source(builder.Bytes())
	if err != nil {
		return err
	}

	return os.WriteFile(fileName, sourceData, os.ModePerm)
}

It's a bit more verbose but a lot more readable from my point of view.

@ldez
Copy link
Contributor

ldez commented Aug 7, 2022

Note: you always say "forgive me", but I think this is not what you want to say.

"forgive me" means "can you forget me", but I don't want to forget you 😄

Maybe you are trying to say: "excuse me" or "sorry"

Edit: maybe it's an English expression 🤔 that I don't know (I'm not native) but it's really surprising.

@sashamelentyev
Copy link
Owner Author

Thank you! I take your suggestions.

Oh, yes, I mean "excuse me". Thank you for English lesson!

"Thank you for all" x 1000

pkg/analyzer/internal/gen.go Outdated Show resolved Hide resolved
pkg/analyzer/internal/gen.go Outdated Show resolved Hide resolved
pkg/analyzer/internal/gen.go Outdated Show resolved Hide resolved
pkg/analyzer/internal/gen.go Outdated Show resolved Hide resolved
pkg/analyzer/internal/gen.go Outdated Show resolved Hide resolved
pkg/analyzer/internal/gen.go Outdated Show resolved Hide resolved
pkg/analyzer/internal/gen.go Outdated Show resolved Hide resolved
pkg/analyzer/internal/gen.go Outdated Show resolved Hide resolved
pkg/analyzer/internal/gen.go Outdated Show resolved Hide resolved
@ldez
Copy link
Contributor

ldez commented Aug 7, 2022

you can pick all the changes in one commit in the "Files changed" tab ("add to batch")

@ldez
Copy link
Contributor

ldez commented Aug 7, 2022

A more general tip: you can choose the kind of merge you want to use on the "merge button".

You can use the "Squash merge" instead of "Create a merge commit".

This means that you don't have to squash/push force all the time.

@sashamelentyev
Copy link
Owner Author

Thank you for tips!

@ldez
Copy link
Contributor

ldez commented Aug 7, 2022

LGTM

@sashamelentyev sashamelentyev merged commit 773f14e into main Aug 7, 2022
@sashamelentyev sashamelentyev deleted the feat/gen branch August 7, 2022 11:52
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