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

Do not omit zero int values on create operations #318

Merged
merged 10 commits into from
Dec 2, 2021
Merged

Do not omit zero int values on create operations #318

merged 10 commits into from
Dec 2, 2021

Conversation

smaeda-ks
Copy link
Contributor

@smaeda-ks smaeda-ks commented Nov 12, 2021 •
edited
Loading

@smaeda-ks
Copy link
Contributor Author

@Integralist Do you think this requires updating test fixtures?

@smaeda-ks
Copy link
Contributor Author

Also, we seem to have some inconsistencies between int vs uint.

@smaeda-ks smaeda-ks marked this pull request as ready for review November 12, 2021 12:11
@Integralist
Copy link
Collaborator

I'd personally prefer to regenerate the fixtures.

I agree there is an inconsistency. I typically use int by default, but I can appreciate that if we know we'll only ever need positive number then using uint makes sense. I wouldn't worry too much about this for now, especially if we consider in the future moving go-fastly to being code-generated.

Integralist
Integralist approved these changes Nov 15, 2021
View reviewed changes
Copy link
Collaborator

@Integralist Integralist left a comment

Choose a reason for hiding this comment

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

Approving ahead of fixtures being regenerated.

@smaeda-ks
Copy link
Contributor Author

@Integralist updated fixtures.

@smaeda-ks
Copy link
Contributor Author

ok to merge? I don't have write access though

@Integralist
Copy link
Collaborator

Oh, sorry @smaeda-ks I'll merge now 😬

@Integralist Integralist merged commit 2106fe1 into fastly:main Dec 2, 2021
@Integralist Integralist added the bug label Dec 3, 2021
@chenrui333 chenrui333 mentioned this pull request Dec 4, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Integralist Integralist Integralist approved these changes

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

Successfully merging this pull request may close these issues.
2 participants
@smaeda-ks @Integralist