Skip to content
This repository was archived by the owner on Jan 18, 2026. It is now read-only.

feat: add enum generator#524

Closed
EduardoLaranjo wants to merge 2 commits intohamba:mainfrom
EduardoLaranjo:main
Closed

feat: add enum generator#524
EduardoLaranjo wants to merge 2 commits intohamba:mainfrom
EduardoLaranjo:main

Conversation

@EduardoLaranjo
Copy link
Copy Markdown

@EduardoLaranjo EduardoLaranjo commented Apr 15, 2025 •
edited
Loading

Goal of this PR

This PR tackles issue #428 by updating the avro generator to generate enums with the following format:

const (
	SPADES   Cards = "SPADES"
	HEARTS   Cards = "HEARTS"
	DIAMONDS Cards = "DIAMONDS"
	CLUBS    Cards = "CLUBS"
)

Instead of the just strings

How did I test it?

Regenerate the golden files

@nrwiersma
Copy link
Copy Markdown
Member

nrwiersma commented Apr 16, 2025

In order to get this reviewed, please fill in the PR description.

@EduardoLaranjo
Copy link
Copy Markdown
Author

EduardoLaranjo commented Apr 16, 2025

Apologies @nrwiersma, I have update the PR description. I understand you are doing some refactor on generator, so this PR might not be needed, let me know.

nrwiersma
nrwiersma reviewed Apr 22, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@nrwiersma nrwiersma left a comment

Choose a reason for hiding this comment

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

Nice change. I would be concerned that this will break some peoples code. IMO, it would be better to put it behind a flag/option to avoid this and maintain the status quo.

@EduardoLaranjo
Copy link
Copy Markdown
Author

EduardoLaranjo commented Apr 23, 2025 •
edited
Loading

Nice change. I would be concerned that this will break some peoples code. IMO, it would be better to put it behind a flag/option to avoid this and maintain the status quo.

Yep I can make as an option. I was wondering if you have any idea to avoid name clashing.

I will also create a dedicated test for this feature

@nrwiersma
Copy link
Copy Markdown
Member

nrwiersma commented Apr 23, 2025

I was wondering if you have any idea to avoid name clashing.

Conventionally speaking, it gets prefixed with the type, which would be singular. Like this:

const (
	CardSpades   Card = "SPADES"
	CardHearts   Card = "HEARTS"
	CardDiamonds Card = "DIAMONDS"
	CardClubs    Card = "CLUBS"
)

@Emptyless Emptyless mentioned this pull request Apr 24, 2025
Merged
@EduardoLaranjo EduardoLaranjo force-pushed the main branch 2 times, most recently from 4ccd9d2 to c1f4590 Compare May 12, 2025 22:55
@EduardoLaranjo
Copy link
Copy Markdown
Author

EduardoLaranjo commented May 12, 2025

hi @nrwiersma I added the enum generation as optional and add one test

nrwiersma
nrwiersma suggested changes May 13, 2025
View reviewed changes
gen/testdata/golden.go Outdated
type Cards string

const (
SPADES Cards = "SPADES"
Copy link
Copy Markdown
Member

@nrwiersma nrwiersma May 13, 2025

Choose a reason for hiding this comment

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

This should not be all upper. It should also be prefixed with the type. So CardSpades.

gen/testdata/golden.go Outdated
"github.com/hamba/avro/v2"
)

type Cards string
Copy link
Copy Markdown
Member

@nrwiersma nrwiersma May 13, 2025

Choose a reason for hiding this comment

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

This should be singular. It does not represent multiple cards, but a card.

@gdbi
Copy link
Copy Markdown

gdbi commented Aug 5, 2025

waiting for this to be merged .. thanks @nrwiersma

@nrwiersma
Copy link
Copy Markdown
Member

nrwiersma commented Aug 6, 2025

Waiting for the review to be addressed.

@slashformotion slashformotion mentioned this pull request Oct 9, 2025
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@nrwiersma nrwiersma nrwiersma requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@EduardoLaranjo @nrwiersma @gdbi