Provide three more custom option for command-line generator #110

dengsh12 · 2024-07-18T19:21:00Z

Proposed changes

Provide -directive-map-name, -match-func-nameand-match-func-comment` options for command-line generator. Allows users to specify the names of generated map and matchFunc, comment for matchFunc

Checklist

Before creating a PR, run through this checklist and mark each as complete.

I have read the CONTRIBUTING document
If applicable, I have added tests that prove my fix is effective or that my feature works
If applicable, I have checked that any relevant tests pass after adding my changes
I have updated any relevant documentation (README.md)

UnwashedMeme · 2024-07-22T13:10:23Z

cmd/generate/main.go

+		directiveMapName  = flag.String("directive-map", "", `Name of the generated map variable.(required)`)
+		matchFuncName     = flag.String("match-func", "", `Name of the generated matchFunc.(required)`)


In the other MR you had mentioned wanting to provide a helpful default here. I like that idea but thought having a wrapper script enforce a variable was provided to be really weird.

If this is can reasonably have a default, and it's just that lua/headers need to provide a value then have those provide the value -- that's exactly what optional args are for.

What was your thinking for switching to required?

Originally I made directive-map and match-func optional just for convenience in this usercase: an user has a small dynamic module and he wants to use crossplane to parse configs contain that module(but we don't want him open a MR to add //go:generate for that module). In this case he can run ./cmd/generate/ directly, and pass the generated MatchFunc to Parseoptions.DirectiveSources and I thought providing default values for these two params save the user's time.

After you mentioned the problem, I really agreed it's quite weird for maintainers to understand why we defined it as required in shell script but optional in ./cmd/generate. And the default values not that make sense. directive-map is the name of the generated map, and match-func is the name of the generated matchFunc. Consider the former usercase, the user is generating a support file for a dynamic module(say it module_a), and the directive-map and match-func should be something relevant to module_a(like aDirectives, MatchADirectives). The default values I provided before don't really make sense, and I'm also concerned optional may cause users to overlook them.

Suggested change

directiveMapName = flag.String("directive-map", "", `Name of the generated map variable.(required)`)

matchFuncName = flag.String("match-func", "", `Name of the generated matchFunc.(required)`)

directiveMapName = flag.String("directive-map", "", `Name of the generated map variable.(required)`)

matchFuncName = flag.String("match-func", "", `Name of the generated matchFunc. (required)`)

ornj · 2024-07-23T20:31:48Z

cmd/generate/main.go

@@ -19,6 +19,8 @@ func main() {
 	var (
 		sourceCodePath = flag.String("src-path", "",
 			"The path of source code your want to generate support from, it can be either a file or a directory. (required)")
+		directiveMapName  = flag.String("directive-map", "", `Name of the generated map variable.(required)`)


Suggested change

directiveMapName = flag.String("directive-map", "", `Name of the generated map variable.(required)`)

directiveMapName = flag.String("directive-map", "", `Name of the generated map variable. (required)`)

Would this be more "self documenting" if the flag was -variable or -variable-name? The fact that the type is a map seems inconsequential.

At the beginning I also used directive-map-name but I'm not sure if it was too long lol. Changed to directive-map-name and match-func-name in latest commit

I think you ignored the bit where "map" isn't really necessary information to communicate, but that's OK I guess.

I'm not sure if you want to do anything to enforce that the resulting variable is unexported or just suggest to the caller that it shouldn't be.

I'm not sure if you want to do anything to enforce that the resulting variable is unexported or just suggest to the caller that it shouldn't be.

I think it's just suggestion? User may really want the variable exported sometimes(e.g. they want to iterate though the map in other package)

Updated: add comments for directive-map-name and match-func-name

ornj · 2024-07-23T20:32:55Z

cmd/generate/main.go

+		directiveMapName  = flag.String("directive-map", "", `Name of the generated map variable.(required)`)
+		matchFuncName     = flag.String("match-func", "", `Name of the generated matchFunc.(required)`)


Suggested change

directiveMapName = flag.String("directive-map", "", `Name of the generated map variable.(required)`)

matchFuncName = flag.String("match-func", "", `Name of the generated matchFunc.(required)`)

directiveMapName = flag.String("directive-map", "", `Name of the generated map variable.(required)`)

matchFuncName = flag.String("match-func", "", `Name of the generated matchFunc. (required)`)

ornj · 2024-07-23T20:35:55Z

internal/generator/generator.go

+type GenerateConfig struct {
+	Filter           map[string]struct{}
+	Override         map[string][]Mask
+	DirectiveMapName string
+	MatchFuncName    string
+}


Nit: it would be good practice to add doc comments to this type to communicate what each of these config settings do.

I see your point. But I thought the variable names here are clear? But it is just from my angle. If you felt unclear about any of them I can add comments. I'm trying to save some comments like this you've sent me before lol https://preview.redd.it/vs35fh1ruef51.jpg?width=640&crop=smart&auto=webp&s=3e1dd46e210f0499b9ac87187eefbde912bebdeb

I think the names are decent, but here's an example that I think adds value.

// DirectiveMapName is the name assigned to the variable containing the directives // discovered in the NGINX module source. The variable name generally starts with a // lowercase to avoid export. Users will use the generated function named by // MatchFuncName to validate the module directives in nginx configurations. DirectiveMapName string

MatchFuncName would similarly be documented to communicate that it should be pascal case to be exported.

Updated: add comments for some fields in GenerateConfig

ornj · 2024-07-23T20:40:17Z

internal/generator/testdata/expected/normalDefinition

+// MyMatchFn is a matchFunc for parsing an NGINX config that contains the
 // preceding directives.


Nit: I think this testdata file is illustrating that the doc comment could be made more informative. As worded, the reader would need to look at the source to figure out what this function is doing.

Perhaps the generated functions are well named so it's not a big deal. Maybe there could be an optional flag that allows for some configuration of the doc string to say which nginx release or module this relates to?

Perhaps the generated functions are well named so it's not a big deal.

Yeah we would name it like LuaDirectivesMatchFn in //go:generate .... This is just test data, not exact dynamic module so the name is kind of not that descriptive

Maybe there could be an optional flag that allows for some configuration of the doc string to say which nginx release or module this relates to?

In go generate we will name the MatchFunc well. The tool will only be called directly when user wants to support some dynamic modules locally without MR. I thought if they want they can write comments directly? It's easier than pass comments to a command-line argument

I thought if they want they can write comments directly? It's easier than pass comments to a command-line argument

You mean edit-- and re-edit-- when the file is generated?

Adding a --comment option is one way to do it. Another way would be to allow the caller to provide a module name or some other string that gets dropped into the template. Forgive the pseudo syntax that follows.

// {{.MatchFuncName}} is a matchFunc for parsing an NGINX config that contains // directives from the {{.ModuleName}} module

You could add some logic to it or format it differently to add a bit more flexibility.

Updated: add match-func-comment option

codecov · 2024-07-24T15:54:14Z

Welcome to Codecov 🎉

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

Thanks for integrating Codecov - We've got you covered ☂️

ornj · 2024-07-24T16:28:23Z

internal/generator/generator.go

+	Filter map[string]struct{}
+
+	Override map[string][]Mask


Nit: I think these could use comments too to at least explain what string is.

Oh sure! These are useful information to add. Added in latest commit

ornj · 2024-07-24T16:29:42Z

internal/generator/generator.go

+	// MatchFuncComment is the comment appears above the generated MatchFunc.
+	// It may contain some information like what modules are included
+	// in the generated MatchFunc.


Should it start with MatchFuncName to follow Go doc comment conventions, or will you auto generate that part of the comment? What happens if this is left empty?

Should it start with MatchFuncName to follow Go doc comment conventions

Sounds good. I added comments suggesting that it should start with MatchFuncName(but not enforced) in latest commit.

What happens if this is left empty?

If it is empty, no code comments for MatchFunc. I added comments for this in latest commit.

… code generator

dengsh12 requested a review from a team as a code owner July 18, 2024 19:21

dengsh12 mentioned this pull request Jul 18, 2024

go generate support for njs/headersmore #109

Closed

4 tasks

UnwashedMeme approved these changes Jul 22, 2024

View reviewed changes

ornj approved these changes Jul 23, 2024

View reviewed changes

dengsh12 changed the title ~~Provide two more custom option for command-line generator~~ Provide three more custom option for command-line generator Jul 24, 2024

ornj reviewed Jul 24, 2024

View reviewed changes

ornj approved these changes Jul 24, 2024

View reviewed changes

add -directive-map-name, -match-func-name, and -match-func-comment to…

82990a0

… code generator

dengsh12 merged commit 282db55 into nginxinc:main Jul 24, 2024
2 checks passed

		directiveMapName = flag.String("directive-map", "", `Name of the generated map variable.(required)`)
		matchFuncName = flag.String("match-func", "", `Name of the generated matchFunc.(required)`)

		// MyMatchFn is a matchFunc for parsing an NGINX config that contains the
		// preceding directives.

Provide three more custom option for command-line generator #110

Provide three more custom option for command-line generator #110

Uh oh!

Conversation

dengsh12 commented Jul 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed changes

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dengsh12 Jul 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Jul 24, 2024

Welcome to Codecov 🎉

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dengsh12 commented Jul 18, 2024 •

edited

Loading

dengsh12 Jul 22, 2024 •

edited

Loading