Skip to content

Adjustment to the function that determines whether to import AvroTypeGen in the code to be generated #141

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.

Sign up for GitHub

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

Closed
thiagodpf wants to merge 2 commits into from
Closed

Adjustment to the function that determines whether to import AvroTypeGen in the code to be generated #141

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
251da19
Update generate.go
thiagodpf Aug 31, 2024
d093b3e
Update generate.go
thiagodpf Sep 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 6 additions & 7 deletions cmd/avrogo/generate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,14 +29,13 @@ const nullType = "avrotypegen.Null"
// schema.RecordDefinition by looking at their match within given parsed namespace
func shouldImportAvroTypeGen(namespace *parser.Namespace, definitions []schema.QualifiedName) bool {
Copy link
Contributor

@rogpeppe rogpeppe Sep 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't looked in details but that == comparison is definitely wrong!

How about something like this instead, which keeps the binary search but (hopefully!) fixes the logic:

// shouldImportAvroTypeGen reports whether avrotypegen is required. It checks that the definitions given are of type
// schema.RecordDefinition by looking at their match within given parsed namespace
func shouldImportAvroTypeGen(namespace *parser.Namespace, definitions []schema.QualifiedName) bool {
	for _, def := range namespace.Definitions {
		searchName := def.AvroName().Name
		_, found := sort.Find(len(definitions), func(i int) int {
			return strings.Compare(searchName, definitions[i].Name)
		})
		if found {
			switch def.(type) {
			case *schema.RecordDefinition, *schema.FixedDefinition:
				return true
			}
		}
	}
	return false
}

Note: I haven't actually run the above code :)

Copy link
Author

@thiagodpf thiagodpf Sep 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It worked perfectly! I'll adjust the PR.

Thanks for the help, I really don't have much experience with the sort package and I was able to learn some cool stuff! 🙇‍♂️

for _, def := range namespace.Definitions {
defToGenerateIdx := sort.Search(len(definitions), func(i int) bool {
return definitions[i].Name == def.AvroName().Name
searchName := def.AvroName().Name
_, found := sort.Find(len(definitions), func(i int) int {
return strings.Compare(searchName, definitions[i].Name)
})
if defToGenerateIdx < len(definitions) && def.AvroName().Name == definitions[defToGenerateIdx].Name {
if _, ok := def.(*schema.RecordDefinition); ok {
return true
}
if _, ok := def.(*schema.FixedDefinition); ok {
if found {
switch def.(type) {
case *schema.RecordDefinition, *schema.FixedDefinition:
return true
}
}
Expand Down