Skip to content

fix issue: createItem not serializing Nothings correctly #81

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.

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

Merged
merged 4 commits into from
Mar 14, 2025

Conversation

jmartinez-sb
Copy link
Contributor

Some fields were reported in the issue as appearing with their values as null, when in fact they shouldn'd have appeared at all in the JSON serialization.

I've included them inside catMaybes so this is corrected. I've also created a couple of tests to determine whether a couple of fields (one of them with Nothing, the other one with a Just value) appear in the JSON serialization. Lastly, I've included a another dependency for the purposes of converting among Text, String, Bytestring and lazy Bytestring mostly for the new tests.

@jmartinez-sb jmartinez-sb self-assigned this Feb 28, 2025
Copy link
Member

@DavidMazarro DavidMazarro left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks for adding tests! I left a couple of comments not entirely related to your fix or the tests, but that should still be addressed.
The CI is failing due to an issue with the GitHub Actions runner not being able to use secrets (i.e. the Rollbar token) in external PRs coming from forks. We investigated this a while ago, this is an intentional limitation to avoid security breaches such as people logging or abusing secrets when creating PRs. I've tested your PR locally and can confirm that the tests are passing 🎉

@DavidMazarro DavidMazarro added dependencies Pull requests that update a dependency file fix This contribution is a fix for a bug labels Mar 11, 2025
@jmartinez-sb
Copy link
Contributor Author

Oh, so the CI failing is something that is expected? Phew, this would've saved me some time trying to solve it, I wish I knew it earlier!
I've applied you suggestions. Take a look when you have some time.

Copy link
Member

@DavidMazarro DavidMazarro left a comment

Choose a reason for hiding this comment

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

LGTM! Good job :)

@DavidMazarro
Copy link
Member

Solves #23.

@DavidMazarro DavidMazarro merged commit 6be58b5 into stackbuilders:main Mar 14, 2025
1 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file fix This contribution is a fix for a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants