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.

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

Jump to bottom

Add optional type to config #59

Merged
merged 1 commit into from
Jul 20, 2024
Merged

Add optional type to config #59

merged 1 commit into from
Jul 20, 2024

Conversation

VolkerLieber
Copy link
Contributor

Some JSON converters (e.g. gofiber JSON) actually convert optional fields to null instead of undefined.
This commit implements an "optional" config option to choose if undefined or null is generated.
Please check if everything is implemented correctly.

gzuidhof
gzuidhof requested changes Jun 10, 2024
View reviewed changes
Copy link
Owner

@gzuidhof gzuidhof left a comment

Choose a reason for hiding this comment

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

Looks great overall, thank you for the contribution!

I left one comment on what I think is a mistake (but I may be wrong?)

tygo/config.go Outdated Show resolved Hide resolved
@gzuidhof
Copy link
Owner

One more nit: perhaps the field name OptionalType / optional_type makes it more obvious what it describes? The Optional field may be for enabling or disabling optional support globally.

@gzuidhof
Copy link
Owner

The CI failing is unrelated - goreleaser had some breaking changes and it's about time the config is updated.

gzuidhof
gzuidhof approved these changes Jun 10, 2024
View reviewed changes
Copy link
Owner

@gzuidhof gzuidhof left a comment

Choose a reason for hiding this comment

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

Thank you for this contribution :)

@VolkerLieber
Copy link
Contributor Author

You're welcome and sorry for the hassle, I did this while being a bit occupied with other tasks.
The next PR will be a bit more carved out

@gzuidhof gzuidhof merged commit 5189d5b into gzuidhof:main Jul 20, 2024
1 check failed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gzuidhof gzuidhof gzuidhof approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@VolkerLieber @gzuidhof